On Mon, Aug 27, 2018 at 1:42 PM Daniel Santos <daniel.santos@xxxxxxxxx> 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@xxxxxxxxxxxxx>
> >>> Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
> >>> Reviewed-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
> >>> ---
> >>>
> >>> 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
