| Message ID | CA+55aFzsrVF7W+YQye=EeF4Wk3yDOTbb_vSiM8s1zkKbE4JV4Q@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 16, 2018 at 8:27 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Fri, Mar 16, 2018 at 10:55 AM, Al Viro <viro@zeniv.linux.org.uk> wrote: >> >> That's not them, that's C standard regarding ICE. > > Yes. The C standard talks about "integer constant expression". I know. > It's come up in this very thread before. > > The C standard at no point talks about - or forbids - "variable length > arrays". That never comes up in the whole standard, I checked. > > So we are right now hindered by a _syntactic_ check, without any way > to have a _semantic_ check. > > That's my problem. The warnings are misleading and imply semantics. > > And apparently there is no way to actually check semantics. > >> 1,100 is *not* a constant expression as far as the standard is concerned, > > I very much know. > > But it sure isn't "variable" either as far as the standard is > concerned, because the standard doesn't even have that concept (it > uses "variable" for argument numbers and for variables). > > So being pedantic doesn't really change anything. > >> Would you argue that in >> void foo(char c) >> { >> int a[(c<<1) + 10 - c + 2 - c]; > > Yeah, I don't think that even counts as a constant value, even if it > can be optimized to one. I would not at all be unhppy to see > __builtin_constant_p() to return zero. > > But that is very much different from the syntax issue. > > So I would like to get some way to get both type-checking and constant > checking without the annoying syntax issue. > >> expr, constant_expression is not a constant_expression. And in >> this particular case the standard is not insane - the only reason >> for using that is typechecking and _that_ can be achieved without >> violating 6.6p6: >> sizeof(expr,0) * 0 + ICE >> *is* an integer constant expression, and it gives you exact same >> typechecking. So if somebody wants to play odd games, they can >> do that just fine, without complicating the logics for compilers... > > Now that actually looks like a good trick. Maybe we can use that > instead of the comma expression that causes problems. > > And using sizeof() to make sure that __builtin_choose_expression() > really gets an integer constant expression and that there should be no > ambiguity looks good. > > Hmm. > > This works for me, and I'm being *very* careful (those casts to > pointer types are inside that sizeof, because it's not an integral > type, and non-integral casts are not valid in an ICE either) but > somebody needs to check gcc-4.4: > > #define __typecheck(a,b) \ > (!!(sizeof((typeof(a)*)1==(typeof(b)*)1))) > > #define __no_side_effects(a,b) \ > (__builtin_constant_p(a)&&__builtin_constant_p(b)) > > #define __safe_cmp(a,b) \ > (__typecheck(a,b) && __no_side_effects(a,b)) > > #define __cmp(a,b,op) ((a)op(b)?(a):(b)) > > #define __cmp_once(a,b,op) ({ \ > typeof(a) __a = (a); \ > typeof(b) __b = (b); \ > __cmp(__a,__b,op); }) > > #define __careful_cmp(a,b,op) \ > __builtin_choose_expr(__safe_cmp(a,b), __cmp(a,b,op), > __cmp_once(a,b,op)) > > #define min(a,b) __careful_cmp(a,b,<) > #define max(a,b) __careful_cmp(a,b,>) > #define min_t(t,a,b) __careful_cmp((t)(a),(t)(b),<) > #define max_t(t,a,b) __careful_cmp((t)(a),(t)(b),>) > > and yes, it does cause new warnings for that > > comparison between ‘enum tis_defaults’ and ‘enum tpm2_const’ > > in drivers/char/tpm/tpm_tis_core.h due to > > #define TIS_TIMEOUT_A_MAX max(TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_A) > > but technically that warning is actually correct, I'm just confused > why gcc cares about the cast placement or something. > > That warning is easy to fix by turning it into a "max_t(int, enum1, > enum2)' and that is technically the right thing to do, it's just not > warned about for some odd reason with the current code. > > Kees - is there some online "gcc-4.4 checker" somewhere? This does > seem to work with my gcc. I actually tested some of those files you > pointed at now. I use this one: https://godbolt.org/ Cheers, Miguel
On Fri, Mar 16, 2018 at 12:27:23PM -0700, Linus Torvalds wrote: > But it sure isn't "variable" either as far as the standard is > concerned, because the standard doesn't even have that concept (it > uses "variable" for argument numbers and for variables). Huh? 6.7.5.2p4: If the size is not present, the array type is an incomplete type. If the size is * instead of being an expression, the array type is a variable length array type of unspecified size, which can only be used in declarations with function prototype scope [footnote]; such arrays are nonetheless complete types. If the size is an integer constant expression and the element type has a known constant size, the array type is not a variable length array type; otherwise, the array type is a variable length array type. footnote: Thus, * can be used only in function declarations that are not definitions (see 6.7.5.3). That's C99, straight from N1256.pdf (C99-TC3)...
On Fri, Mar 16, 2018 at 1:03 PM, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: >> >> Kees - is there some online "gcc-4.4 checker" somewhere? This does >> seem to work with my gcc. I actually tested some of those files you >> pointed at now. > > I use this one: > > https://godbolt.org/ Well, my *test* code works on that one and -Wvla -Werror. It does not work with gcc-4.1.x, but works with gcc-4.4.x. I can't seem to see the errors any way, I wonder if __builtin_choose_expr() simply didn't exist back then. Odd that you can't view warnings/errors with it. But it's possible that it fails on more complex stuff in the kernel. I've done a "allmodconfig" build with that patch, and the only issue it found was that (real) type issue in tpm_tis_core.h. Linus
On Fri, Mar 16, 2018 at 1:12 PM, Al Viro <viro@zeniv.linux.org.uk> wrote: > > That's C99, straight from N1256.pdf (C99-TC3)... I checked C90, since the error is ISO C90 forbids variable length array and I didn't see anything there. Admittedly I only found a draft copy. Linus
On Fri, Mar 16, 2018 at 01:15:27PM -0700, Linus Torvalds wrote: > On Fri, Mar 16, 2018 at 1:12 PM, Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > That's C99, straight from N1256.pdf (C99-TC3)... > > I checked C90, since the error is > > ISO C90 forbids variable length array > > and I didn't see anything there. Well, yes - VLA are new in C99; C90 never had those...
On Fri, Mar 16, 2018 at 1:14 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > It does not work with gcc-4.1.x, but works with gcc-4.4.x. > > I can't seem to see the errors any way, I wonder if > __builtin_choose_expr() simply didn't exist back then. No, that goes further back. It seems to be -Wvla itself that doesn't exist in 4.1, so the test build failed simply because I used that flag ;) Linus
On Fri, Mar 16, 2018 at 9:14 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Fri, Mar 16, 2018 at 1:03 PM, Miguel Ojeda > <miguel.ojeda.sandonis@gmail.com> wrote: >>> >>> Kees - is there some online "gcc-4.4 checker" somewhere? This does >>> seem to work with my gcc. I actually tested some of those files you >>> pointed at now. >> >> I use this one: >> >> https://godbolt.org/ > > Well, my *test* code works on that one and -Wvla -Werror. > > It does not work with gcc-4.1.x, but works with gcc-4.4.x. > > I can't seem to see the errors any way, I wonder if > __builtin_choose_expr() simply didn't exist back then. > > Odd that you can't view warnings/errors with it. Just click on the button left/bottom of the compilation window. Cheers, Miguel
On Fri, Mar 16, 2018 at 12:27 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > Kees - is there some online "gcc-4.4 checker" somewhere? This does > seem to work with my gcc. I actually tested some of those files you > pointed at now. Unfortunately my 4.4 test fails quickly: ./include/linux/jiffies.h: In function ‘jiffies_delta_to_clock_t’: ./include/linux/jiffies.h:444: error: first argument to ‘__builtin_choose_expr’ not a constant static inline clock_t jiffies_delta_to_clock_t(long delta) { return jiffies_to_clock_t(max(0L, delta)); } I think this is the same problem of using __builtin_constant_p() in 4.4 that we hit earlier? :( -Kees
On Sat, Mar 17, 2018 at 12:27 AM, Kees Cook <keescook@chromium.org> wrote: > > Unfortunately my 4.4 test fails quickly: > > ./include/linux/jiffies.h: In function ‘jiffies_delta_to_clock_t’: > ./include/linux/jiffies.h:444: error: first argument to > ‘__builtin_choose_expr’ not a constant Ok, so it really looks like that same "__builtin_constant_p() doesn't return a constant". Which is really odd, but there you have it. I wonder if you can use that "sizeof()" to force evaluation of it, because sizeof() really does end up being magical when it comes to "integer constant expression". So instead of this: #define __no_side_effects(a,b) \ (__builtin_constant_p(a)&&__builtin_constant_p(b)) that just assumes that __builtin_constant_p() itself always counts as a constant expression, what happens if you do #define __is_constant(a) \ (sizeof(char[__builtin_constant_p(a)])) #define __no_side_effects(a,b) \ (__is_constant(a) && __is_constant(b)) I realize that the above looks completely insane: the whole point is to *not* have VLA's, and we know that __builtin_constant_p() isn't always evaliated as a constant. But hear me out: if the issue is that there's some evaluation ordering between the two builtins, and the problem is that the __builtin_choose_expr() part of the expression is expanded *before* the __builtin_constant_p() has been expanded, then just hiding it inside that bat-shit-crazy sizeof() will force that to be evaluated first (because a sizeof() is defined to be a integer constant expression. So the above is completely insane, bit there is actually a chance that using that completely crazy "x -> sizeof(char[x])" conversion actually helps, because it really does have a (very odd) evaluation-time change. sizeof() has to be evaluated as part of the constant expression evaluation, in ways that "__builtin_constant_p()" isn't specified to be done. But it is also definitely me grasping at straws. If that doesn't work for 4.4, there's nothing else I can possibly see. Linus
On Sat, Mar 17, 2018 at 11:52 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > So the above is completely insane, bit there is actually a chance that > using that completely crazy "x -> sizeof(char[x])" conversion actually > helps, because it really does have a (very odd) evaluation-time > change. sizeof() has to be evaluated as part of the constant > expression evaluation, in ways that "__builtin_constant_p()" isn't > specified to be done. > > But it is also definitely me grasping at straws. If that doesn't work > for 4.4, there's nothing else I can possibly see. No luck! :( gcc 4.4 refuses to play along. And, hilariously, not only does it not change the complaint about __builtin_choose_expr(), it also thinks that's a VLA now. ./include/linux/mm.h: In function ‘get_mm_hiwater_rss’: ./include/linux/mm.h:1567: warning: variable length array is used ./include/linux/mm.h:1567: error: first argument to ‘__builtin_choose_expr’ not a constant 6.8 is happy with it (of course). I do think the earlier version (without the sizeof-hiding-builting_constant_p) provides a template for a const_max() that both you and Rasmus would be happy with, though! -Kees
On Sat, Mar 17, 2018 at 01:07:32PM -0700, Kees Cook wrote: > On Sat, Mar 17, 2018 at 11:52 AM, Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > So the above is completely insane, bit there is actually a chance that > > using that completely crazy "x -> sizeof(char[x])" conversion actually > > helps, because it really does have a (very odd) evaluation-time > > change. sizeof() has to be evaluated as part of the constant > > expression evaluation, in ways that "__builtin_constant_p()" isn't > > specified to be done. > > > > But it is also definitely me grasping at straws. If that doesn't work > > for 4.4, there's nothing else I can possibly see. > > No luck! :( gcc 4.4 refuses to play along. And, hilariously, not only > does it not change the complaint about __builtin_choose_expr(), it > also thinks that's a VLA now. > > ./include/linux/mm.h: In function ‘get_mm_hiwater_rss’: > ./include/linux/mm.h:1567: warning: variable length array is used > ./include/linux/mm.h:1567: error: first argument to > ‘__builtin_choose_expr’ not a constant > > 6.8 is happy with it (of course). > > I do think the earlier version (without the > sizeof-hiding-builting_constant_p) provides a template for a > const_max() that both you and Rasmus would be happy with, though! I thought we were dropping support for 4.4 (for other reasons). Isn't it 4.6 we should be looking at?
On 2018-03-17 19:52, Linus Torvalds wrote: > On Sat, Mar 17, 2018 at 12:27 AM, Kees Cook <keescook@chromium.org> wrote: >> >> Unfortunately my 4.4 test fails quickly: >> >> ./include/linux/jiffies.h: In function ‘jiffies_delta_to_clock_t’: >> ./include/linux/jiffies.h:444: error: first argument to >> ‘__builtin_choose_expr’ not a constant > > Ok, so it really looks like that same "__builtin_constant_p() doesn't > return a constant". > > Which is really odd, but there you have it. Not really. We do rely on builtin_constant_p not being folded too quickly to a 0/1 answer, so that gcc still generates good code even if the argument is only known to be constant at a late(r) optimization stage (through inlining and all). So unlike types_compatible_p, which can obviously be answered early during parsing, builtin_constant_p is most of the time a yes/no/maybe/it's complicated thing. Sure, when the argument is just a literal or perhaps even any kind of ICE, gcc can fold it to "yes", and I think it does (though the details of when and if gcc does that can obviously be very version-dependent, which may be some of what we've seen). But when it's not that obvious, gcc leaves it in the undetermined state. That's not good enough for builtin_choose_expr, because even the type of the resulting expression depends on that first argument, so that really must be resolved early. So to have some kind of builtin_constant_p control a builtin_choose_expr, it would need to be a "builtin_ice_p" or "builtin_obviously_constant_p" that would always be folded to 0/1 as part of evaluating ICEs. So I don't think there's any way around creating a separate macro for use with compile-time constants. Rasmus
On Sun, Mar 18, 2018 at 2:13 PM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > On 2018-03-17 19:52, Linus Torvalds wrote: >> >> Ok, so it really looks like that same "__builtin_constant_p() doesn't >> return a constant". >> >> Which is really odd, but there you have it. > > Not really. We do rely on builtin_constant_p not being folded too > quickly to a 0/1 answer, so that gcc still generates good code even if > the argument is only known to be constant at a late(r) optimization > stage (through inlining and all). Hmm. That makes sense. It just doesn't work for our case when we really want to choose the expression based on side effects or not. > So unlike types_compatible_p, which > can obviously be answered early during parsing, builtin_constant_p is > most of the time a yes/no/maybe/it's complicated thing. The silly thing is, the thing we *really* want to know _is_ actually easily accessible during the early parsing, exactly like __builtin_compatible_p(): it's not really that we care about the expressions being constant, as much as the "can this have side effects" question. We only really use __builtin_constant_p() as a (bad) approximation of that in this case, since it's the best we can do. So the real use would be to say "can we use the simple direct macro that just happens to also fold into a nice integer constant expression" for __builtin_choose_expr(). I tried to find something like that, but it really doesn't exist, even though I would actually have expected it to be a somewhat common concern for macro writers: write a macro that works in any arbitrary environment. I guess array sizes are really the only true limiting environment (static initializers is another one). How annoying. Odd that newer gcc's seem to do so much better (ie gcc-5 seems to be fine). So _something_ must have changed. Linus
On 2018-03-18 22:33, Linus Torvalds wrote: > On Sun, Mar 18, 2018 at 2:13 PM, Rasmus Villemoes > <linux@rasmusvillemoes.dk> wrote: >> On 2018-03-17 19:52, Linus Torvalds wrote: >>> >>> Ok, so it really looks like that same "__builtin_constant_p() doesn't >>> return a constant". >>> >>> Which is really odd, but there you have it. >> >> Not really. We do rely on builtin_constant_p not being folded too >> quickly to a 0/1 answer, so that gcc still generates good code even if >> the argument is only known to be constant at a late(r) optimization >> stage (through inlining and all). > > Hmm. That makes sense. It just doesn't work for our case when we > really want to choose the expression based on side effects or not. > >> So unlike types_compatible_p, which >> can obviously be answered early during parsing, builtin_constant_p is >> most of the time a yes/no/maybe/it's complicated thing. > > The silly thing is, the thing we *really* want to know _is_ actually > easily accessible during the early parsing, exactly like > __builtin_compatible_p(): it's not really that we care about the > expressions being constant, as much as the "can this have side > effects" question. OK, I missed where this was made about side effects of x and y, but I suppose the idea was to use no_side_effects(x) && no_side_effects(y) ? trivial_max(x, y) : old_max(x, y) or the same thing spelled with b_c_e? Yes, I think that would work, if we indeed had that way of checking an expression. > We only really use __builtin_constant_p() as a (bad) approximation of > that in this case, since it's the best we can do. I don't think you should parenthesize bad, rather capitalize it. ({x++; 0;}) is constant in the eyes of __builtin_constant_p, but not side-effect free. Sure, that's very contrived, but I can easily imagine some max(f(foo), -1) call where f is sometimes an external function, but for other .configs it's a static inline that always returns 0, but still has some non-trivial side-effect before that. And this would all depend on which optimizations gcc applies before it decides to evaluate builtin_constant_p, so could be some fun debugging. Good thing that that didn't work out... > So the real use would be to say "can we use the simple direct macro > that just happens to also fold into a nice integer constant > expression" for __builtin_choose_expr(). > > I tried to find something like that, but it really doesn't exist, even > though I would actually have expected it to be a somewhat common > concern for macro writers: write a macro that works in any arbitrary > environment. Yeah, I think the closest is a five year old suggestion (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57612) to add a __builtin_assert_no_side_effects, that could be used in macros that for some reason cannot be implemented without evaluating some argument multiple times. It would be more useful to have the predicate version, which one could always turn into a build bug version. But we have neither, unfortunately. Rasmus
On Sun, Mar 18, 2018 at 3:59 PM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > > OK, I missed where this was made about side effects of x and y We never made it explicit, since all we really cared about in the end is the constantness. But yes: > but I suppose the idea was to use > > no_side_effects(x) && no_side_effects(y) ? trivial_max(x, y) : > old_max(x, y) Exactly. Except with __builtin_choose_expr(), because we need the end result to be seen as a integer constant expression, so that we can then use it as an array size. So it needs that early parse-time resolution. >> We only really use __builtin_constant_p() as a (bad) approximation of >> that in this case, since it's the best we can do. > > I don't think you should parenthesize bad, rather capitalize it. ({x++; > 0;}) is constant in the eyes of __builtin_constant_p, but not > side-effect free. Hmm. Yeah, but probably don't much care for the kernel. For example, we do things like this: #define __swab64(x) \ (__builtin_constant_p((__u64)(x)) ? \ ___constant_swab64(x) : \ __fswab64(x)) where that "___constant_swab64()" very much uses the same argument over and over. And we do that for related reasons - we really want to do the constant folding at build time for some cases, and this was the only sane way to do it. Eg code like return (addr & htonl(0xff000000)) == htonl(0x7f000000); wants to do the right thing, and long ago gcc didn't have builtins for byte swapping, so we had to just do nasty horribly macros that DTRT for constants. But since the kernel is standalone, we don't need to really worry about the *generic* case, we just need to worry about our own macros, and if somebody does that example you show I guess we'll just have to shun them ;) Of course, our own macros are often macros from hell, exactly because they often contain a lot of type-checking and/or type-(size)-based polymorphism. But then we actually *want* gcc to simplify things, and avoid side effects, just have potentially very complex expressions. But we basically never have that kind of intentionally evil macros, so we are willing to live with a bad substitute. But yes, it would be better to have some more control over things, and actually have a way to say "if X is a constant integer expression, do transformation Y, otherwise call function y()". Actually sparse started out with the idea in the background that it could become not just a checker, but a "transformation tool". Partly because of long gone historical issues (ie gcc people used to be very anti-plugin due to licensing issues). Of course, a more integrated and powerful preprocessor language is almost always what we *really* wanted, but traditionally "powerful preprocessor" has always meant "completely incomprehensible and badly integrated preprocessor". "cpp" may be a horrid language, but it's there and it's fast (when integrated with the front-end, like everybody does now) But sadly, cpp is really bad for the above kind of "if argument is constant" kind of tricks. I suspect we'd use it a lot otherwise. >> So the real use would be to say "can we use the simple direct macro >> that just happens to also fold into a nice integer constant >> expression" for __builtin_choose_expr(). >> >> I tried to find something like that, but it really doesn't exist, even >> though I would actually have expected it to be a somewhat common >> concern for macro writers: write a macro that works in any arbitrary >> environment. > > Yeah, I think the closest is a five year old suggestion > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57612) to add a > __builtin_assert_no_side_effects, that could be used in macros that for > some reason cannot be implemented without evaluating some argument > multiple times. It would be more useful to have the predicate version, > which one could always turn into a build bug version. But we have > neither, unfortunately. Yeah, and since we're in the situation that *new* gcc versions work for us anyway, and we only have issues with older gcc's (that sadly people still use), even if there was a new cool feature we couldn't use it. Oh well. Thanks for the background. Linus
From: linus971@gmail.com [mailto:linus971@gmail.com] On Behalf Of Linus Torvalds > Sent: 18 March 2018 23:36 ... > > Yeah, and since we're in the situation that *new* gcc versions work > for us anyway, and we only have issues with older gcc's (that sadly > people still use), even if there was a new cool feature we couldn't > use it. Is it necessary to have the full checks for old versions of gcc? Even -Wvla could be predicated on very recent gcc - since we aren't worried about whether gcc decides to generate a vla, but whether the source requests one. David
On Mon, Mar 19, 2018 at 2:43 AM, David Laight <David.Laight@aculab.com> wrote: > > Is it necessary to have the full checks for old versions of gcc? > > Even -Wvla could be predicated on very recent gcc - since we aren't > worried about whether gcc decides to generate a vla, but whether > the source requests one. You are correct. We could just ignore the issue with old gcc versions, and disable -Wvla rather than worry about it. Linus
On Sat, Mar 17, 2018 at 1:07 PM, Kees Cook <keescook@chromium.org> wrote: > > No luck! :( gcc 4.4 refuses to play along. And, hilariously, not only > does it not change the complaint about __builtin_choose_expr(), it > also thinks that's a VLA now. Hmm. So thanks to the diseased mind of Martin Uecker, there's a better test for "__is_constant()": /* Glory to Martin Uecker <Martin.Uecker@med.uni-goettingen.de> */ #define __is_constant(a) \ (sizeof(int) == sizeof(*(1 ? ((void*)((a) * 0l)) : (int*)1))) that is actually *specified* by the C standard to work, and doesn't even depend on any gcc extensions. The reason is some really subtle pointer conversion rules, where the type of the ternary operator will depend on whether one of the pointers is NULL or not. And the definition of NULL, in turn, very much depends on "integer constant expression that has the value 0". Are you willing to do one final try on a generic min/max? Same as my last patch, but using the above __is_constant() test instead of __builtin_constant_p? Linus
On Tue, Mar 20, 2018 at 4:23 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Hmm. So thanks to the diseased mind of Martin Uecker, there's a better > test for "__is_constant()": > > /* Glory to Martin Uecker <Martin.Uecker@med.uni-goettingen.de> */ > #define __is_constant(a) \ > (sizeof(int) == sizeof(*(1 ? ((void*)((a) * 0l)) : (int*)1))) > > that is actually *specified* by the C standard to work, and doesn't > even depend on any gcc extensions. Well, it does depend on 'sizeof(*(void *)X)' being 1 and the compiler not complaining about it, and that sizeof(int) is not 1. But since we depend on those things in the kernel anyway, that's fine. Linus
On Tue, Mar 20, 2018 at 04:26:52PM -0700, Linus Torvalds wrote: > On Tue, Mar 20, 2018 at 4:23 PM, Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > Hmm. So thanks to the diseased mind of Martin Uecker, there's a better > > test for "__is_constant()": > > > > /* Glory to Martin Uecker <Martin.Uecker@med.uni-goettingen.de> */ > > #define __is_constant(a) \ > > (sizeof(int) == sizeof(*(1 ? ((void*)((a) * 0l)) : (int*)1))) > > > > that is actually *specified* by the C standard to work, and doesn't > > even depend on any gcc extensions. > > Well, it does depend on 'sizeof(*(void *)X)' being 1 and the compiler > not complaining about it, and that sizeof(int) is not 1. > > But since we depend on those things in the kernel anyway, that's fine. It also depends upon "ICE for null pointer constant purposes" having the same extensions as "ICE for enum purposes", etc., which is not obvious. Back in 2007 or so we had a long thread regarding null pointer constants in sparse; I probably still have notes from back then, but that'll take some serious digging to find ;-/ What's more, gcc definitely has odd extensions. Example I remember from back then: extern unsigned n; struct { int x : 1 + n - n; } y; is accepted. Used to be quietly accepted with -Wpedantic -std=c99, even, but that got fixed - with -Wpedantic it does, at least, warn. What is and what is not recognized is fairly random - 1 + n - n + 1 + n - n is recognized as "constant", 1 + n + n + 1 - n - n is not. Of course, neither is an ICE.
On Tue, Mar 20, 2018 at 4:23 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Sat, Mar 17, 2018 at 1:07 PM, Kees Cook <keescook@chromium.org> wrote: >> >> No luck! :( gcc 4.4 refuses to play along. And, hilariously, not only >> does it not change the complaint about __builtin_choose_expr(), it >> also thinks that's a VLA now. > > Hmm. So thanks to the diseased mind of Martin Uecker, there's a better > test for "__is_constant()": > > /* Glory to Martin Uecker <Martin.Uecker@med.uni-goettingen.de> */ > #define __is_constant(a) \ > (sizeof(int) == sizeof(*(1 ? ((void*)((a) * 0l)) : (int*)1))) > > that is actually *specified* by the C standard to work, and doesn't > even depend on any gcc extensions. I feel we risk awakening Cthulhu with this. :) > The reason is some really subtle pointer conversion rules, where the > type of the ternary operator will depend on whether one of the > pointers is NULL or not. > > And the definition of NULL, in turn, very much depends on "integer > constant expression that has the value 0". > > Are you willing to do one final try on a generic min/max? Same as my > last patch, but using the above __is_constant() test instead of > __builtin_constant_p? So, this time it's not a catastrophic failure with gcc 4.4. Instead it fails in 11 distinct places: $ grep "first argument to ‘__builtin_choose_expr’ not a constant" log | cut -d: -f1-2 crypto/ablkcipher.c:71 crypto/blkcipher.c:70 crypto/skcipher.c:95 mm/percpu.c:2453 net/ceph/osdmap.c:1545 net/ceph/osdmap.c:1756 net/ceph/osdmap.c:1763 mm/kmemleak.c:1371 mm/kmemleak.c:1403 drivers/infiniband/hw/hfi1/pio_copy.c:421 drivers/infiniband/hw/hfi1/pio_copy.c:547 Seems like it doesn't like void * arguments: mm/percpu.c: void *ptr; ... base = min(ptr, base); mm/kmemleak.c: static void scan_large_block(void *start, void *end) ... next = min(start + MAX_SCAN_SIZE, end); I'll poke a bit more... -Kees
From: Kees Cook > Sent: 22 March 2018 15:01 ... > > /* Glory to Martin Uecker <Martin.Uecker@med.uni-goettingen.de> */ > > #define __is_constant(a) \ > > (sizeof(int) == sizeof(*(1 ? ((void*)((a) * 0l)) : (int*)1))) ... > So, this time it's not a catastrophic failure with gcc 4.4. Instead it > fails in 11 distinct places: ... > Seems like it doesn't like void * arguments: > > mm/percpu.c: > void *ptr; > ... > base = min(ptr, base); Try adding (unsigned long) before the (a). David
On Thu, Mar 22, 2018 at 8:01 AM, Kees Cook <keescook@chromium.org> wrote: > > Seems like it doesn't like void * arguments: Yeah, that was discussed separately, I just didn't realize we had any such users. As David said, just adding a (long) cast to it should be fine, ie #define __is_constant(a) \ (sizeof(int) == sizeof(*(1 ? ((void*)((long)(a) * 0l)) : (int*)1))) and is probably a good idea even outside of pointers (because "long long" constants could cause warnings too due to the cast to (void *)). Linus
include/linux/kernel.h | 77 +++++++++++++------------------------------------- 1 file changed, 20 insertions(+), 57 deletions(-) diff --git a/include/linux/kernel.h b/include/linux/kernel.h index 3fd291503576..23c31bf1d7fb 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -787,37 +787,29 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } * strict type-checking.. See the * "unnecessary" pointer comparison. */ -#define __min(t1, t2, min1, min2, x, y) ({ \ - t1 min1 = (x); \ - t2 min2 = (y); \ - (void) (&min1 == &min2); \ - min1 < min2 ? min1 : min2; }) +#define __typecheck(a,b) \ + (!!(sizeof((typeof(a)*)1==(typeof(b)*)1))) -/** - * min - return minimum of two values of the same or compatible types - * @x: first value - * @y: second value - */ -#define min(x, y) \ - __min(typeof(x), typeof(y), \ - __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \ - x, y) +#define __no_side_effects(a,b) \ + (__builtin_constant_p(a)&&__builtin_constant_p(b)) -#define __max(t1, t2, max1, max2, x, y) ({ \ - t1 max1 = (x); \ - t2 max2 = (y); \ - (void) (&max1 == &max2); \ - max1 > max2 ? max1 : max2; }) +#define __safe_cmp(a,b) \ + (__typecheck(a,b) && __no_side_effects(a,b)) -/** - * max - return maximum of two values of the same or compatible types - * @x: first value - * @y: second value - */ -#define max(x, y) \ - __max(typeof(x), typeof(y), \ - __UNIQUE_ID(max1_), __UNIQUE_ID(max2_), \ - x, y) +#define __cmp(a,b,op) ((a)op(b)?(a):(b)) + +#define __cmp_once(a,b,op) ({ \ + typeof(a) __a = (a); \ + typeof(b) __b = (b); \ + __cmp(__a,__b,op); }) + +#define __careful_cmp(a,b,op) \ + __builtin_choose_expr(__safe_cmp(a,b), __cmp(a,b,op), __cmp_once(a,b,op)) + +#define min(a,b) __careful_cmp(a,b,<) +#define max(a,b) __careful_cmp(a,b,>) +#define min_t(t,a,b) __careful_cmp((t)(a),(t)(b),<) +#define max_t(t,a,b) __careful_cmp((t)(a),(t)(b),>) /** * min3 - return minimum of three values @@ -856,35 +848,6 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } */ #define clamp(val, lo, hi) min((typeof(val))max(val, lo), hi) -/* - * ..and if you can't take the strict - * types, you can specify one yourself. - * - * Or not use min/max/clamp at all, of course. - */ - -/** - * min_t - return minimum of two values, using the specified type - * @type: data type to use - * @x: first value - * @y: second value - */ -#define min_t(type, x, y) \ - __min(type, type, \ - __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \ - x, y) - -/** - * max_t - return maximum of two values, using the specified type - * @type: data type to use - * @x: first value - * @y: second value - */ -#define max_t(type, x, y) \ - __max(type, type, \ - __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \ - x, y) - /** * clamp_t - return a value clamped to a given range using a given type * @type: the type of variable to use