improve constexpr handling
Subsystems (2)
cover improve constexpr handling
include/linux/cpumask.h +1 -1
@@ -736,7 +736,7 @@ void init_cpu_online(const struct cpumask *src);*/ #define to_cpumask (bitmap) \ ((struct cpumask *)(1 ? (bitmap) \ - : (void *)sizeof(__check_is_bitmap(bitmap)))) + : (void *)0 + sizeof(__check_is_bitmap(bitmap))))static inline int __check_is_bitmap (const unsigned long *bitmap) {
include/linux/log2.h +84 -88
190 lines not on this page · raw message
include/uapi/linux/swab.h +12 -12
@@ -102,28 +102,28 @@ static inline __attribute_const__ __u32 __fswahb32(__u32 val)* __swab16 - return a byteswapped 16-bit value * @x: value to byteswap */ -#define __swab16 (x) \ - (__builtin_constant_p((__u16)(x)) ? \ - ___constant_swab16(x) : \ - __fswab16(x)) +#define __swab16 (x) \ + __builtin_choose_expr(__builtin_constant_p((__u16)(x)), \ + ___constant_swab16(x), \ + __fswab16(x))/** * __swab32 - return a byteswapped 32-bit value * @x: value to byteswap */ -#define __swab32 (x) \ - (__builtin_constant_p((__u32)(x)) ? \ - ___constant_swab32(x) : \ - __fswab32(x)) +#define __swab32 (x) \ + __builtin_choose_expr(__builtin_constant_p((__u32)(x)), \ + ___constant_swab32(x), \ + __fswab32(x))/** * __swab64 - return a byteswapped 64-bit value * @x: value to byteswap */ -#define __swab64 (x) \ - (__builtin_constant_p((__u64)(x)) ? \ - ___constant_swab64(x) : \ - __fswab64(x)) +#define __swab64 (x) \ + __builtin_choose_expr(__builtin_constant_p((__u64)(x)), \ + ___constant_swab64(x), \ + __fswab64(x))/** * __swahw32 - return a word-swapped 32-bit value
This patch series is split into four parts:
- The first part deals with the refactorization of the current integer constant expression handling and introduces some support for recognizing arithmetic expressions. [1-5/13]
- The second part introduces support for recognizing address constants. [6/13]
- The third part introduces a check for the constness of static storage duration objects' initializers. [7/13]
- The last part stems from my tests with the kernel. It contains things I missed in the first [9-10/13] and second [8,12/13] parts and relaxes some of the constraints on constant expressions [11/13]. For the last patch [13/13], please see below.
My initial intent was to rework the current integer constant expression handling in order to allow for the recognition of constant subexpressions built up by means of __builtin_choose_expr(). Hence the first part.
However, since I had to touch the whole constant expression handling code anyways, I decided to experimentally extend it to support arithmetic constant expressions and address constants as well. Hence the second part.
Since the additional information on expressions obtained through the first two parts is rather pointless without making any use of it, I implemented part three, the checking of static storage duration objects' initializers for constness.
This part is the reason why there is a 'RFC' tag in the subject. It is up to you to decide whether letting sparse check for C99 conformity is a valuable thing to have or whether being stricter than GCC is counter-productive/completely idiotic.
Although the patches of the fourth part, the fixup part, fit very well into the first two categories, their associated testcases, if any, depend on [7/13]. Thus, I dediced to keep the order as is.
If you decide that you do not want to have the third part, I will resend a stripped version of the first one only, together with [9-10/13].
Random notes:
- [1/13] is basically a renaming patch only. It introduces additional flags used in later patches. It should not alter any behaviour.
- I am not very confident about [13/13]. It could introduce some new issues as well as just fix symptoms, not the problem. Please double check when reviewing.
- This series introduces two new testsuite failers, namely validation/choose_expr.c and validation/builtin_bswap.c. These testcases really violate the constexpr constraints. However, for the moment, I left them as is.
Results from running sparse on the kernel:
For testing, I ran sparse without and with this series applied on an allmodconfig kernel on x86_64.
Beforehand, I fixed commonly used macros, namely log2- and swab-related ones and to_cpumask() to qualify as constexprs, if possible. See the attached diff (and blame me for attaching diffs). Especially log2() needs this fixing anyways since it is used all over the place where only integer constant expressions are allowed (in array designators).
sparse now finds 519 occurences of non-const initializers of static storage duration objects, 474 of which are located in drivers/acpi and stem from this subsystem's custom offsetof macro implemented by means of taking pointer differences.
The remaining hits are distributed as follows
14 drivers/net/ethernet/sfc/
13 drivers/isdn/hardware/mISDN/
8 drivers/usb/core/
2 drivers/net/wireless/libertas_tf/
1 drivers/infiniband/hw/qib/
1 drivers/misc/genwqe/
1 drivers/net/ethernet/qlogic/qlcnic/
1 drivers/net/wireless/ath/ath6kl/
1 drivers/scsi/snic/
1 drivers/staging/comedi/drivers/
1 drivers/tty/ipwireless/
1 net/wireless/
OTOH, 40 errors due to non-integer constant expressions in array designators formerly spit out by sparse have vanished. This is because the attached diff on log2() relies on __builtin_warning() getting recognized as an integer constant, i.e. on [10/13].
Nicolai Stange (13):
expression: introduce additional expression constness tracking flags
expression: examine constness of casts at evaluation only
expression: examine constness of binops and alike at evaluation only
expression: examine constness of preops at evaluation only
expression: examine constness of conditionals at evaluation only expression, evaluate: add support for recognizing address constants
evaluate: check static storage duration objects' intializers'
constness
expression: recognize references to labels as address constants
expression: examine constness of __builtin_offsetof at evaluation only
symbol: flag builtins constant_p, safe_p and warning as constexprs
evaluate: relax some constant expression rules for pointer expressions expression, evaluate: support compound literals as address constants
symbol: do not inherit storage modifiers from base types at
examination
evaluate.c | 191 +++++++++++++++++++++++++------- expand.c | 2 +- expression.c | 54 +++++---- expression.h | 133 +++++++++++++++++++++- symbol.c | 12 +- validation/constexpr-binop.c | 33 ++++++ validation/constexpr-cast.c | 25 +++++ validation/constexpr-compound-literal.c | 18 +++ validation/constexpr-conditional.c | 34 ++++++ validation/constexpr-init.c | 109 ++++++++++++++++++ validation/constexpr-offsetof.c | 21 ++++ validation/constexpr-preop.c | 29 +++++ 12 files changed, 578 insertions(+), 83 deletions(-) create mode 100644 validation/constexpr-binop.c create mode 100644 validation/constexpr-cast.c create mode 100644 validation/constexpr-compound-literal.c create mode 100644 validation/constexpr-conditional.c create mode 100644 validation/constexpr-init.c create mode 100644 validation/constexpr-offsetof.c create mode 100644 validation/constexpr-preop.c
Signature · 1 line
2.4.5
[Side note: for some reason, your mail had your message ordered *after* your attached diff, so replies quote the diff before the message.]
Quoted 9 lines · Nicolai Stange
My initial intent was to rework the current integer constant expression handling in order to allow for the recognition of constant subexpressions built up by means of
__builtin_choose_expr(). Hence the first part.However, since I had to touch the whole constant expression handling code anyways, I decided to experimentally extend it to support arithmetic constant expressions and address constants as well. Hence the second part.
Quoted 8 lines
Since the additional information on expressions obtained through the first two parts is rather pointless without making any use of it, I implemented part three, the checking of static storage duration objects' initializers for constness.
This part is the reason why there is a 'RFC' tag in the subject. It is up to you to decide whether letting sparse check for C99 conformity is a valuable thing to have or whether being stricter than GCC is counter-productive/completely idiotic.
I think it's absolutely a valuable thing to have. It may or may not be the right *default* behavior, but having an appropriate -W option to enable it would be a good start.
I've seen kernel maintainers ask people to not rely on GCC's lax enforcement of constant initializers.
Quoted 4 lines
sparse now finds 519 occurences of non-const initializers of static storage duration objects, 474 of which are located in
drivers/acpiand stem from this subsystem's custom offsetof macro implemented by means of taking pointer differences.
Ideally, I'd suggest that ACPICA should add a translation from ACPI_OFFSET to offsetof as part of its Linux-izing scripts.
That said, I also can't think of an obvious reason why ACPI_OFFSET *should* be considered non-constant. Perhaps there's a detail in the C99 spec that explains why what it does isn't OK, but it *seems* like it should be a compile-time constant expression. I've CCed Al Viro, who knows the C99 constant expression rules very well; Al, could you provide some clarity here? The ACPI_OFFSET macro in question expands to this:
(acpi_size) (((u8 *) (void *) ((&(((struct some_struct *) 0)->fieldname)))) - ((u8 *) (void *) (((void *) (void *) 0))))
Does the -> make this non-constant?
- Josh Triplett
Quoted 2 lines · [email protected] writes:
[Side note: for some reason, your mail had your message ordered *after* your attached diff, so replies quote the diff before the message.]
Yes, I messed it up for some reason. Sorry for that.
Quoted 9 lines
Quoted 8 lines
Since the additional information on expressions obtained through the first two parts is rather pointless without making any use of it, I implemented part three, the checking of static storage duration objects' initializers for constness.
This part is the reason why there is a 'RFC' tag in the subject. It is up to you to decide whether letting sparse check for C99 conformity is a valuable thing to have or whether being stricter than GCC is counter-productive/completely idiotic.
Quoted 3 lines
I think it's absolutely a valuable thing to have. It may or may not be the right *default* behavior, but having an appropriate -W option to enable it would be a good start.
My next resend will contain such a -Wcheck-static-initializers then. However, I will delay that resend in order to be able to incorporate other reviews arriving in the meantime.
Quoted 16 lines · Nicolai Stange
Quoted 4 lines
sparse now finds 519 occurences of non-const initializers of static storage duration objects, 474 of which are located in
drivers/acpiand stem from this subsystem's custom offsetof macro implemented by means of taking pointer differences.Ideally, I'd suggest that ACPICA should add a translation from
ACPI_OFFSETto offsetof as part of its Linux-izing scripts.That said, I also can't think of an obvious reason why
ACPI_OFFSET*should* be considered non-constant. Perhaps there's a detail in the C99 spec that explains why what it does isn't OK, but it *seems* like it should be a compile-time constant expression. I've CCed Al Viro, who knows the C99 constant expression rules very well; Al, could you provide some clarity here? TheACPI_OFFSETmacro in question expands to this:
Quoted 3 lines
(
acpi_size) (((u8 *) (void *) ((&(((structsome_struct*) 0)->fieldname)))) - ((u8 *) (void *) (((void *) (void *) 0))))Does the -> make this non-constant?
No, it is the pointer difference. At least to my interpretion of C99 [6.6(9)] which might be arguable.
Upon your request, I could relax these constraints as I have did already for some special cases of conditionals in [11/13].
Quoted 10 lines · Nicolai Stange
Quoted 9 lines · [email protected] writes:
Quoted 8 lines
Since the additional information on expressions obtained through the first two parts is rather pointless without making any use of it, I implemented part three, the checking of static storage duration objects' initializers for constness.
This part is the reason why there is a 'RFC' tag in the subject. It is up to you to decide whether letting sparse check for C99 conformity is a valuable thing to have or whether being stricter than GCC is counter-productive/completely idiotic.
Quoted 5 lines
Quoted 3 lines
I think it's absolutely a valuable thing to have. It may or may not be the right *default* behavior, but having an appropriate -W option to enable it would be a good start.
My next resend will contain such a -Wcheck-static-initializers then.
<bikeshed>Shouldn't it be something like -Wnon-constant-initializer, since that's what it checks for?</bikeshed>
Quoted 2 lines
However, I will delay that resend in order to be able to incorporate other reviews arriving in the meantime.
Sounds reasonable.
Quoted 16 lines
Quoted 16 lines · Nicolai Stange
Quoted 4 lines
sparse now finds 519 occurences of non-const initializers of static storage duration objects, 474 of which are located in
drivers/acpiand stem from this subsystem's custom offsetof macro implemented by means of taking pointer differences.Ideally, I'd suggest that ACPICA should add a translation from
ACPI_OFFSETto offsetof as part of its Linux-izing scripts.That said, I also can't think of an obvious reason why
ACPI_OFFSET*should* be considered non-constant. Perhaps there's a detail in the C99 spec that explains why what it does isn't OK, but it *seems* like it should be a compile-time constant expression. I've CCed Al Viro, who knows the C99 constant expression rules very well; Al, could you provide some clarity here? TheACPI_OFFSETmacro in question expands to this:
Quoted 8 lines
Quoted 3 lines
(
acpi_size) (((u8 *) (void *) ((&(((structsome_struct*) 0)->fieldname)))) - ((u8 *) (void *) (((void *) (void *) 0))))Does the -> make this non-constant?
No, it is the pointer difference. At least to my interpretion of C99 [6.6(9)] which might be arguable.
Upon your request, I could relax these constraints as I have did already for some special cases of conditionals in [11/13].
Ah, I see. I don't know whether relaxing that makes sense or not; Al?
- Josh Triplett
Quoted 43 lines · [email protected] writes:
Quoted 10 lines · Nicolai Stange
Quoted 9 lines · [email protected] writes:
Quoted 8 lines
Since the additional information on expressions obtained through the first two parts is rather pointless without making any use of it, I implemented part three, the checking of static storage duration objects' initializers for constness.
This part is the reason why there is a 'RFC' tag in the subject. It is up to you to decide whether letting sparse check for C99 conformity is a valuable thing to have or whether being stricter than GCC is counter-productive/completely idiotic.Quoted 5 lines
Quoted 3 lines
I think it's absolutely a valuable thing to have. It may or may not be the right *default* behavior, but having an appropriate -W option to enable it would be a good start.
My next resend will contain such a -Wcheck-static-initializers then.
<bikeshed>Shouldn't it be something like -Wnon-constant-initializer, since that's what it checks for?</bikeshed>
Quoted 2 lines
However, I will delay that resend in order to be able to incorporate other reviews arriving in the meantime.
Sounds reasonable.
Quoted 18 lines
Quoted 18 lines · Nicolai Stange
Quoted 4 lines
sparse now finds 519 occurences of non-const initializers of static storage duration objects, 474 of which are located in
drivers/acpiand stem from this subsystem's custom offsetof macro implemented by means of taking pointer differences.Ideally, I'd suggest that ACPICA should add a translation from
ACPI_OFFSETto offsetof as part of its Linux-izing scripts.That said, I also can't think of an obvious reason why
ACPI_OFFSET*should* be considered non-constant. Perhaps there's a detail in the C99 spec that explains why what it does isn't OK, but it *seems* like it should be a compile-time constant expression. I've CCed Al Viro, who knows the C99 constant expression rules very well; Al, could you provide some clarity here? TheACPI_OFFSETmacro in question expands to this:(
acpi_size) (((u8 *) (void *) ((&(((structsome_struct*) 0)->fieldname)))) - ((u8 *) (void *) (((void *) (void *) 0))))
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
Does the -> make this non-constant?
No, it is the pointer difference. At least to my interpretion of C99 [6.6(9)] which might be arguable.
Upon your request, I could relax these constraints as I have did already for some special cases of conditionals in [11/13].Ah, I see. I don't know whether relaxing that makes sense or not; Al?
Just a friendly ping to get some more reviews to consider in my next resend, especially on [13/13] and whether to relax the constant expression rules to treat pointer differences of address constants as (arithmetic or integer) constant expressions.
Christopher Li
maintainerQuoted 4 lines · Nicolai Stange
Just a friendly ping to get some more reviews to consider in my next resend, especially on [13/13] and whether to relax the constant expression rules to treat pointer differences of address constants as (arithmetic or integer) constant expressions.
Sorry for the late reply. I am working on it. It will take me a bit of time to think about the change. I like the generate direction you are going.
Chris
Quoted 10 lines · Christopher Li
Quoted 4 lines · Nicolai Stange
Just a friendly ping to get some more reviews to consider in my next resend, especially on [13/13] and whether to relax the constant expression rules to treat pointer differences of address constants as (arithmetic or integer) constant expressions.
Sorry for the late reply. I am working on it. It will take me a bit of time to think about the change. I like the generate direction you are going.
Chris
Hi Chris,
do you see any way to get this one going again?
Cheers,
Nicolai
Quoted 22 lines · On Wed, Jul 22, 2015 at 05:37:57PM -0700, [email protected] wrote:
[Side note: for some reason, your mail had your message ordered *after* your attached diff, so replies quote the diff before the message.]
Quoted 9 lines · Nicolai Stange
My initial intent was to rework the current integer constant expression handling in order to allow for the recognition of constant subexpressions built up by means of
__builtin_choose_expr(). Hence the first part.However, since I had to touch the whole constant expression handling code anyways, I decided to experimentally extend it to support arithmetic constant expressions and address constants as well. Hence the second part.
Quoted 8 lines
Since the additional information on expressions obtained through the first two parts is rather pointless without making any use of it, I implemented part three, the checking of static storage duration objects' initializers for constness.
This part is the reason why there is a 'RFC' tag in the subject. It is up to you to decide whether letting sparse check for C99 conformity is a valuable thing to have or whether being stricter than GCC is counter-productive/completely idiotic.
Quoted 6 lines
I think it's absolutely a valuable thing to have. It may or may not be the right *default* behavior, but having an appropriate -W option to enable it would be a good start.
I've seen kernel maintainers ask people to not rely on GCC's lax enforcement of constant initializers.
I also think it's a very valuable thing to have.
After all, it's the raison d'etre of sparse to make stricter checks than the standard or GCC.
But then I wonder what's must be done for things like GCC's builtins? Shouldn't, for example, __builtin_bswap32(..) always propagte the constantness of it's argument or it specifically this sort of things that are the target of this patch serie?
Regards,
Luc,
Quoted 27 lines · Luc Van Oostenryck
Quoted 26 lines · On Wed, Jul 22, 2015 at 05:37:57PM -0700, [email protected] wrote:
[Side note: for some reason, your mail had your message ordered *after* your attached diff, so replies quote the diff before the message.]
Quoted 9 lines · Nicolai Stange
My initial intent was to rework the current integer constant expression handling in order to allow for the recognition of constant subexpressions built up by means of
__builtin_choose_expr(). Hence the first part.However, since I had to touch the whole constant expression handling code anyways, I decided to experimentally extend it to support arithmetic constant expressions and address constants as well. Hence the second part.
Quoted 8 lines
Since the additional information on expressions obtained through the first two parts is rather pointless without making any use of it, I implemented part three, the checking of static storage duration objects' initializers for constness.
This part is the reason why there is a 'RFC' tag in the subject. It is up to you to decide whether letting sparse check for C99 conformity is a valuable thing to have or whether being stricter than GCC is counter-productive/completely idiotic.I think it's absolutely a valuable thing to have. It may or may not be the right *default* behavior, but having an appropriate -W option to enable it would be a good start.
Quoted 6 lines
Quoted 2 lines
I've seen kernel maintainers ask people to not rely on GCC's lax enforcement of constant initializers.
I also think it's a very valuable thing to have.
After all, it's the raison d'etre of sparse to make stricter checks than the standard or GCC.
First of all, thank you very much for your review, Luc!
Quoted 5 lines
But then I wonder what's must be done for things like GCC's builtins? Shouldn't, for example,
__builtin_bswap32(..)always propagte the constantness of it's argument or it specifically this sort of things that are the target of this patch serie?
Hmm. I guess it depends on the particular __builtin_*() thingie at hand. In general, unless explicitly documented, I personally would neither assume consistent constness rules nor that those are stable across GCC releases and architectures.
In the case of __builtin_bswap32(..), the kernel seems to follow that line of reasoning: for example in include/uapi/linux/swab.h, we have
#define __swab32(x) \
(__builtin_constant_p((__u32)(x)) ? \
___constant_swab32(x) : \
__fswab32(x))where __fswap32(..) is essentially just a wrapper around __builtin_bswap32(..).
But of course, if one decides that some __builtin_foo(<constexpr>) is a constexpr again, we would have to teach it sparse explicitly. AFAICS, this is nothing new introduced by this patch series though.
Quoted 29 lines · Nicolai Stange
Quoted 27 lines · Luc Van Oostenryck
Quoted 26 lines · On Wed, Jul 22, 2015 at 05:37:57PM -0700, [email protected] wrote:
[Side note: for some reason, your mail had your message ordered *after* your attached diff, so replies quote the diff before the message.]
Quoted 9 lines · Nicolai Stange
My initial intent was to rework the current integer constant expression handling in order to allow for the recognition of constant subexpressions built up by means of
__builtin_choose_expr(). Hence the first part.However, since I had to touch the whole constant expression handling code anyways, I decided to experimentally extend it to support arithmetic constant expressions and address constants as well. Hence the second part.
Quoted 8 lines
Since the additional information on expressions obtained through the first two parts is rather pointless without making any use of it, I implemented part three, the checking of static storage duration objects' initializers for constness.
This part is the reason why there is a 'RFC' tag in the subject. It is up to you to decide whether letting sparse check for C99 conformity is a valuable thing to have or whether being stricter than GCC is counter-productive/completely idiotic.I think it's absolutely a valuable thing to have. It may or may not be the right *default* behavior, but having an appropriate -W option to enable it would be a good start.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
I've seen kernel maintainers ask people to not rely on GCC's lax enforcement of constant initializers.
I also think it's a very valuable thing to have.
After all, it's the raison d'etre of sparse to make stricter checks than the standard or GCC.First of all, thank you very much for your review, Luc!
I'm glad to help to make things move on.
Quoted 10 lines
Quoted 5 lines
But then I wonder what's must be done for things like GCC's builtins? Shouldn't, for example,
__builtin_bswap32(..)always propagte the constantness of it's argument or it specifically this sort of things that are the target of this patch serie?Hmm. I guess it depends on the particular __builtin_*() thingie at hand. In general, unless explicitly documented, I personally would neither assume consistent constness rules nor that those are stable across GCC releases and architectures.
Yes, indeed.
Quoted 8 lines
In the case of
__builtin_bswap32(..), the kernel seems to follow that line of reasoning: for example ininclude/uapi/linux/swab.h, we have#define __swab32(x) \ (__builtin_constant_p((__u32)(x)) ? \ ___constant_swab32(x) : \ __fswab32(x))
Quoted 7 lines
where
__fswap32(..)is essentially just a wrapper around__builtin_bswap32(..).But of course, if one decides that some
__builtin_foo(<constexpr>)is a constexpr again, we would have to teach it sparse explicitly. AFAICS, this is nothing new introduced by this patch series though.
Yes, sure. I was wondering if it is wanted or not to do so.
Yours,
Luc