Date:   Mon, 27 Aug 2018 14:01:36 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     daniel.santos@...ox.com
Cc:     Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Kees Cook <keescook@...omium.org>, sparse@...isli.org,
        linux-sparse@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] compiler.h: give up __compiletime_assert_fallback()

On Mon, Aug 27, 2018 at 1:42 PM Daniel Santos <daniel.santos@...ox.com> wrote:
>
> Hello Nick,
>
> On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
> >> Now we're back to the question of "what do you mean by 'constant'"?  If
> >> you mean a C constant expression (as defined in the C standard) than
> >> almost none of this code fits that criteria.  For these compile-time
> >> assertions to work, we are concerned with the data flow analysis and
> >> constant propagation performed by the compiler during optimization.  You
> >> will notice in include/linux/compiler.h that __compiletime_assert is a
> >> no-op when __OPTIMIZE__ is not defined.
> > Depending on optimizations for static assertions sounds problematic.
>
> (with my best Palpatine voice) It is unavoidable.

LOL

>
> Actually it's theoretically possible, but the compiler would have to do
> something akin to copying it's control flow graph et. al, run -O2-ish
> optimizations, perform the static assertions and then throw away the
> optimized control flow graph and emit code based upon the original.
>
>
> >>> As the comment in <linux/build_bug.h>
> >>> says, GCC (and Clang as well) only emits the error for obvious cases.
> >>>
> >>> When '__cond' is a variable,
> >>>
> >>>     ((void)sizeof(char[1 - 2 * __cond]))
> >>>
> >>> ... is not obvious for the compiler to know the array size is negative.
> >>>
> >>> Reverting that commit would break BUILD_BUG() because negative-size-array
> >>> is evaluated before the code is optimized out.
> >>>
> >>> Let's give up __compiletime_assert_fallback().  This commit does not
> >>> change the current behavior since it just rips off the useless code.
> >> Clang is not the only target audience of
> >> __compiletime_assert_fallback().  Instead of ripping out something that
> >> may benefit builds with gcc 4.2 and earlier, why not override its
> > Note that with commit cafa0010cd51 ("Raise the minimum required gcc
> > version to 4.6") that gcc < 4.6 is irrelevant.
>
> Ah, I guess I'm not keeping up, that's wonderful news!  Considering that
> I guess I would be OK with its removal, but I still think it would be
> better if a similar mechanism to break the Clang build could be found.
>
> >> definition in compiler-clang.h with something that will break the build
> >> for Clang?  It would need an #ifndef __compiletime_error_fallback here
> >> though.
> >>
> >>> Signed-off-by: Masahiro Yamada <yamada.masahiro@...ionext.com>
> >>> Reviewed-by: Kees Cook <keescook@...omium.org>
> >>> Reviewed-by: Nick Desaulniers <ndesaulniers@...gle.com>
> >>> ---
> >>>
> >>> Changes in v2:
> >>>   - Rebase
> >>>
> >>>  include/linux/compiler.h | 17 +----------------
> >>>  1 file changed, 1 insertion(+), 16 deletions(-)
> >>>
> >>> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> >>> index 681d866..87c776c 100644
> >>> --- a/include/linux/compiler.h
> >>> +++ b/include/linux/compiler.h
> >>> @@ -314,29 +314,14 @@ static inline void *offset_to_ptr(const int *off)
> >>>  #endif
> >>>  #ifndef __compiletime_error
> >>>  # define __compiletime_error(message)
> >>> -/*
> >>> - * Sparse complains of variable sized arrays due to the temporary variable in
> >>> - * __compiletime_assert. Unfortunately we can't just expand it out to make
> >>> - * sparse see a constant array size without breaking compiletime_assert on old
> >>> - * versions of GCC (e.g. 4.2.4), so hide the array from sparse altogether.
> >>> - */
> >>> -# ifndef __CHECKER__
> >>> -#  define __compiletime_error_fallback(condition) \
> >>> -     do { ((void)sizeof(char[1 - 2 * condition])); } while (0)
> >>> -# endif
> >>> -#endif
> >>> -#ifndef __compiletime_error_fallback
> >>> -# define __compiletime_error_fallback(condition) do { } while (0)
> >>>  #endif
> >>>
> >>>  #ifdef __OPTIMIZE__
> >>>  # define __compiletime_assert(condition, msg, prefix, suffix)                \
> >>>       do {                                                            \
> >>> -             int __cond = !(condition);                              \
> >>>               extern void prefix ## suffix(void) __compiletime_error(msg); \
> >>> -             if (__cond)                                             \
> >>> +             if (!(condition))                                       \
> >>>                       prefix ## suffix();                             \
> >>> -             __compiletime_error_fallback(__cond);                   \
> >>>       } while (0)
> >>>  #else
> >>>  # define __compiletime_assert(condition, msg, prefix, suffix) do { } while (0)
> >> To give any more meaningful feedback I think I will need to experiment
> >> with Clang, older GCC versions and icc.  It occurred to me that I should
> >> probably clean up and publish my __builtin_constant_p test program and
> >> also generate results for more recent compilers.  I can extend it to
> >> test various negative sized array constructs and it could help inform
> >> this decision.
> >>
> >> IMO, the most ideal solution would be a set of C2x (or future)
> >> extensions providing something similar to C++'s constexpr, GCC's
> >> __builtin_constant_p and our BUILD_BUG_ON.  This would cross deeply into
> > Note that __builtin_constant_p is a wild beast with lots of edge
> > cases, and dependencies on compiler and optimization level.  I'm
> > trying to sort out some of these differences right now with llvm
> > developers.
>
> Yes it is.  IIRC, I even found cases where __builtin_constant_p returned
> false, but the emitted code replaced the value in question with a
> constant.  So at least at one point, constant propagation and
> __builtin_constant_p weren't always entirely coherent.

What?! Do you have a link to a repro on godbolt or a gcc bugreport?

Here's a different case I found that looks problematic:
https://godbolt.org/z/g_iqwh

>
> I was working on a GCC extension that would solve this problem earlier
> this year but bills and paying work interrupted me. :)  The solution
> involved adding a "const" attribute for function parameters and
> variables that would simply create a warning or error if the value
> wasn't compiled away at the time middle-end optimizations completed and
> RTL emitted.  It isn't finished -- maybe for gcc10.
>

Speaking with a coworker internally now, discussing Clang's
diagnose_if/enable_if attributes.  It seems that _Static_assert always
(between compilers) considers parameters non-ICE, which sounds like a
defect to me.

-- 
Thanks,
~Nick Desaulniers

Powered by blists - more mailing lists