| Message ID | 20210505211047.1496765-8-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | misc: Remove variable-length arrays on the stack | expand |
On Wed, May 05, 2021 at 11:10:31PM +0200, Philippe Mathieu-Daudé wrote: > The compiler isn't clever enough to figure 'SEG_CHUNK_SIZE' is > a constant! Help it by using a definitions instead. I don't understand. It's labeled 'const', so any reasonable compiler will place it in the 'text' segment of the executable rather than on the stack. While that's compiler specific, is there really a compiler doing something bad with this? If not, I do prefer the 'const' here if only because it limits the symbol scope ('static const' is also preferred if that helps). > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/block/nvme.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > index 5fe082ec34c..2f6d4925826 100644 > --- a/hw/block/nvme.c > +++ b/hw/block/nvme.c > @@ -812,7 +812,7 @@ static uint16_t nvme_map_sgl(NvmeCtrl *n, NvmeSg *sg, NvmeSglDescriptor sgl, > * descriptors and segment chain) than the command transfer size, so it is > * not bounded by MDTS. > */ > - const int SEG_CHUNK_SIZE = 256; > +#define SEG_CHUNK_SIZE 256 > > NvmeSglDescriptor segment[SEG_CHUNK_SIZE], *sgld, *last_sgld; > uint64_t nsgld; > -- > 2.26.3 >
+Eric On 5/5/21 11:22 PM, Keith Busch wrote: > On Wed, May 05, 2021 at 11:10:31PM +0200, Philippe Mathieu-Daudé wrote: >> The compiler isn't clever enough to figure 'SEG_CHUNK_SIZE' is >> a constant! Help it by using a definitions instead. > > I don't understand. Neither do I TBH... > It's labeled 'const', so any reasonable compiler > will place it in the 'text' segment of the executable rather than on the > stack. While that's compiler specific, is there really a compiler doing > something bad with this? If not, I do prefer the 'const' here if only > because it limits the symbol scope ('static const' is also preferred if > that helps). Using: gcc version 10.2.1 20201125 (Red Hat 10.2.1-9) (GCC) Both static+const / const trigger: hw/block/nvme.c: In function ‘nvme_map_sgl’: hw/block/nvme.c:818:5: error: ISO C90 forbids variable length array ‘segment’ [-Werror=vla] 818 | NvmeSglDescriptor segment[SEG_CHUNK_SIZE], *sgld, *last_sgld; | ^~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> hw/block/nvme.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/block/nvme.c b/hw/block/nvme.c >> index 5fe082ec34c..2f6d4925826 100644 >> --- a/hw/block/nvme.c >> +++ b/hw/block/nvme.c >> @@ -812,7 +812,7 @@ static uint16_t nvme_map_sgl(NvmeCtrl *n, NvmeSg *sg, NvmeSglDescriptor sgl, >> * descriptors and segment chain) than the command transfer size, so it is >> * not bounded by MDTS. >> */ >> - const int SEG_CHUNK_SIZE = 256; >> +#define SEG_CHUNK_SIZE 256 >> >> NvmeSglDescriptor segment[SEG_CHUNK_SIZE], *sgld, *last_sgld; >> uint64_t nsgld; >> -- >> 2.26.3 >> >
On 5/5/21 5:07 PM, Philippe Mathieu-Daudé wrote: > +Eric > > On 5/5/21 11:22 PM, Keith Busch wrote: >> On Wed, May 05, 2021 at 11:10:31PM +0200, Philippe Mathieu-Daudé wrote: >>> The compiler isn't clever enough to figure 'SEG_CHUNK_SIZE' is >>> a constant! Help it by using a definitions instead. >> >> I don't understand. > > Neither do I TBH... > >> It's labeled 'const', so any reasonable compiler >> will place it in the 'text' segment of the executable rather than on the >> stack. While that's compiler specific, is there really a compiler doing >> something bad with this? If not, I do prefer the 'const' here if only >> because it limits the symbol scope ('static const' is also preferred if >> that helps). > > Using: gcc version 10.2.1 20201125 (Red Hat 10.2.1-9) (GCC) > > Both static+const / const trigger: > > hw/block/nvme.c: In function ‘nvme_map_sgl’: > hw/block/nvme.c:818:5: error: ISO C90 forbids variable length array > ‘segment’ [-Werror=vla] > 818 | NvmeSglDescriptor segment[SEG_CHUNK_SIZE], *sgld, *last_sgld; > | ^~~~~~~~~~~~~~~~~ > cc1: all warnings being treated as errors C99 6.7.5.2 paragraph 4: "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." 6.6 paragraph 6: "An integer constant expression shall have integer type and shall only have operands that are integer constants, enumeration constants, character constants, sizeof expressions whose results are integer constants, and floating constants that are the immediate operands of casts. Cast operators in an integer constant expression shall only convert arithmetic types to integer types, except as part of an operand to the sizeof operator." Notably absent from that list are 'const int' variables, which even though they act as constants (in that the name always represents the same value), do not actually qualify as such under C99 rules. Yes, it's a pain. What's more, 6.6 paragraph 10: "An implementation may accept other forms of constant expressions." which means it _should_ be possible for the compiler to do what we want. But just because it is permitted does not make it actually work. :( And while C17 expands the list of integer constant expressions to include _Alignof expressions, it does not add any wording to permit const variables. https://stackoverflow.com/questions/62354105/why-is-const-int-x-5-not-a-constant-expression-in-c helps with this explanation: "The thing to remember (and yes, this is a bit counterintuitive) is that const doesn't mean constant. A constant expression is, roughly, one that can be evaluated at compile time (like 2+2 or 42). The const type qualifier, even though its name is obviously derived from the English word "constant", really means "read-only". Consider, for example, that these are a perfectly valid declarations: const int r = rand(); const time_t now = time(NULL); The const just means that you can't modify the value of r or now after they've been initialized. Those values clearly cannot be determined until execution time." And C++ _does_ support named constants, but we're using C, not C++.
On Wed, May 5, 2021, 5:10 PM Eric Blake <eblake@redhat.com> wrote: > On 5/5/21 5:07 PM, Philippe Mathieu-Daudé wrote: > > +Eric > > > > On 5/5/21 11:22 PM, Keith Busch wrote: > >> On Wed, May 05, 2021 at 11:10:31PM +0200, Philippe Mathieu-Daudé wrote: > >>> The compiler isn't clever enough to figure 'SEG_CHUNK_SIZE' is > >>> a constant! Help it by using a definitions instead. > >> > >> I don't understand. > > > > Neither do I TBH... > > > >> It's labeled 'const', so any reasonable compiler > >> will place it in the 'text' segment of the executable rather than on the > >> stack. While that's compiler specific, is there really a compiler doing > >> something bad with this? If not, I do prefer the 'const' here if only > >> because it limits the symbol scope ('static const' is also preferred if > >> that helps). > > > > Using: gcc version 10.2.1 20201125 (Red Hat 10.2.1-9) (GCC) > > > > Both static+const / const trigger: > > > > hw/block/nvme.c: In function ‘nvme_map_sgl’: > > hw/block/nvme.c:818:5: error: ISO C90 forbids variable length array > > ‘segment’ [-Werror=vla] > > 818 | NvmeSglDescriptor segment[SEG_CHUNK_SIZE], *sgld, *last_sgld; > > | ^~~~~~~~~~~~~~~~~ > > cc1: all warnings being treated as errors > > C99 6.7.5.2 paragraph 4: > "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." > > 6.6 paragraph 6: > "An integer constant expression shall have integer type and shall only > have operands that are integer constants, enumeration constants, > character constants, sizeof expressions whose results are integer > constants, and floating constants that are the immediate operands of > casts. Cast operators in an integer constant expression shall only > convert arithmetic types to integer types, except as part of an operand > to the sizeof operator." > > Notably absent from that list are 'const int' variables, which even > though they act as constants (in that the name always represents the > same value), do not actually qualify as such under C99 rules. Yes, it's > a pain. What's more, 6.6 paragraph 10: > > "An implementation may accept other forms of constant expressions." > > which means it _should_ be possible for the compiler to do what we want. > But just because it is permitted does not make it actually work. :( > > And while C17 expands the list of integer constant expressions to > include _Alignof expressions, it does not add any wording to permit > const variables. > > > https://stackoverflow.com/questions/62354105/why-is-const-int-x-5-not-a-constant-expression-in-c > helps with this explanation: > "The thing to remember (and yes, this is a bit counterintuitive) is that > const doesn't mean constant. A constant expression is, roughly, one that > can be evaluated at compile time (like 2+2 or 42). The const type > qualifier, even though its name is obviously derived from the English > word "constant", really means "read-only". > > Consider, for example, that these are a perfectly valid declarations: > > const int r = rand(); > const time_t now = time(NULL); > > The const just means that you can't modify the value of r or now after > they've been initialized. Those values clearly cannot be determined > until execution time." > > And C++ _does_ support named constants, but we're using C, not C++. > Enum is as close as it gets in C if you are eschewing #define. Warner
On Wed, May 05, 2021 at 06:09:10PM -0500, Eric Blake wrote: > On 5/5/21 5:07 PM, Philippe Mathieu-Daudé wrote: > > +Eric > > > > On 5/5/21 11:22 PM, Keith Busch wrote: > >> On Wed, May 05, 2021 at 11:10:31PM +0200, Philippe Mathieu-Daudé wrote: > >>> The compiler isn't clever enough to figure 'SEG_CHUNK_SIZE' is > >>> a constant! Help it by using a definitions instead. > >> > >> I don't understand. > > > > Neither do I TBH... > > > >> It's labeled 'const', so any reasonable compiler > >> will place it in the 'text' segment of the executable rather than on the > >> stack. While that's compiler specific, is there really a compiler doing > >> something bad with this? If not, I do prefer the 'const' here if only > >> because it limits the symbol scope ('static const' is also preferred if > >> that helps). > > > > Using: gcc version 10.2.1 20201125 (Red Hat 10.2.1-9) (GCC) > > > > Both static+const / const trigger: > > > > hw/block/nvme.c: In function ‘nvme_map_sgl’: > > hw/block/nvme.c:818:5: error: ISO C90 forbids variable length array > > ‘segment’ [-Werror=vla] > > 818 | NvmeSglDescriptor segment[SEG_CHUNK_SIZE], *sgld, *last_sgld; > > | ^~~~~~~~~~~~~~~~~ > > cc1: all warnings being treated as errors > > C99 6.7.5.2 paragraph 4: > "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." > > 6.6 paragraph 6: > "An integer constant expression shall have integer type and shall only > have operands that are integer constants, enumeration constants, > character constants, sizeof expressions whose results are integer > constants, and floating constants that are the immediate operands of > casts. Cast operators in an integer constant expression shall only > convert arithmetic types to integer types, except as part of an operand > to the sizeof operator." > > Notably absent from that list are 'const int' variables, which even > though they act as constants (in that the name always represents the > same value), do not actually qualify as such under C99 rules. Yes, it's > a pain. What's more, 6.6 paragraph 10: > > "An implementation may accept other forms of constant expressions." > > which means it _should_ be possible for the compiler to do what we want. > But just because it is permitted does not make it actually work. :( Thank you, that makes sense. In this case, we are talking about an integer constant expression for the value of a 'const' symbol. That seems like perfect fodder for a compiler to optimize. I understand it's not obligated to do that, but I assumed it would. Anyway, Philippe, I have no objection if you want to push forward with this series.
On May 5 23:10, Philippe Mathieu-Daudé wrote: >The compiler isn't clever enough to figure 'SEG_CHUNK_SIZE' is >a constant! Help it by using a definitions instead. > >Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >--- > hw/block/nvme.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/hw/block/nvme.c b/hw/block/nvme.c >index 5fe082ec34c..2f6d4925826 100644 >--- a/hw/block/nvme.c >+++ b/hw/block/nvme.c >@@ -812,7 +812,7 @@ static uint16_t nvme_map_sgl(NvmeCtrl *n, NvmeSg *sg, NvmeSglDescriptor sgl, > * descriptors and segment chain) than the command transfer size, so it is > * not bounded by MDTS. > */ >- const int SEG_CHUNK_SIZE = 256; >+#define SEG_CHUNK_SIZE 256 > > NvmeSglDescriptor segment[SEG_CHUNK_SIZE], *sgld, *last_sgld; > uint64_t nsgld; >-- >2.26.3 > > Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
On 5/6/21 4:15 AM, Keith Busch wrote: > On Wed, May 05, 2021 at 06:09:10PM -0500, Eric Blake wrote: >> On 5/5/21 5:07 PM, Philippe Mathieu-Daudé wrote: >>> +Eric >>> >>> On 5/5/21 11:22 PM, Keith Busch wrote: >>>> On Wed, May 05, 2021 at 11:10:31PM +0200, Philippe Mathieu-Daudé wrote: >>>>> The compiler isn't clever enough to figure 'SEG_CHUNK_SIZE' is >>>>> a constant! Help it by using a definitions instead. >>>> >>>> I don't understand. >>> >>> Neither do I TBH... >>> >>>> It's labeled 'const', so any reasonable compiler >>>> will place it in the 'text' segment of the executable rather than on the >>>> stack. While that's compiler specific, is there really a compiler doing >>>> something bad with this? If not, I do prefer the 'const' here if only >>>> because it limits the symbol scope ('static const' is also preferred if >>>> that helps). >>> >>> Using: gcc version 10.2.1 20201125 (Red Hat 10.2.1-9) (GCC) >>> >>> Both static+const / const trigger: >>> >>> hw/block/nvme.c: In function ‘nvme_map_sgl’: >>> hw/block/nvme.c:818:5: error: ISO C90 forbids variable length array >>> ‘segment’ [-Werror=vla] >>> 818 | NvmeSglDescriptor segment[SEG_CHUNK_SIZE], *sgld, *last_sgld; >>> | ^~~~~~~~~~~~~~~~~ >>> cc1: all warnings being treated as errors >> >> C99 6.7.5.2 paragraph 4: >> "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." >> >> 6.6 paragraph 6: >> "An integer constant expression shall have integer type and shall only >> have operands that are integer constants, enumeration constants, >> character constants, sizeof expressions whose results are integer >> constants, and floating constants that are the immediate operands of >> casts. Cast operators in an integer constant expression shall only >> convert arithmetic types to integer types, except as part of an operand >> to the sizeof operator." >> >> Notably absent from that list are 'const int' variables, which even >> though they act as constants (in that the name always represents the >> same value), do not actually qualify as such under C99 rules. Yes, it's >> a pain. What's more, 6.6 paragraph 10: >> >> "An implementation may accept other forms of constant expressions." >> >> which means it _should_ be possible for the compiler to do what we want. >> But just because it is permitted does not make it actually work. :( > > Thank you, that makes sense. In this case, we are talking about an > integer constant expression for the value of a 'const' symbol. That > seems like perfect fodder for a compiler to optimize. I understand it's > not obligated to do that, but I assumed it would. > > Anyway, Philippe, I have no objection if you want to push forward with > this series. Thanks both. I'll amend Eric explanation in the commit description. Regards, Phil.
On 5/5/21 2:10 PM, Philippe Mathieu-Daudé wrote: > The compiler isn't clever enough to figure 'SEG_CHUNK_SIZE' is > a constant! Help it by using a definitions instead. This isn't about being clever or not, it's semantics. In C++, "const int" is a proper constant, but in C it is a variable with a constant value. Thus the use of the symbol in the array bounds is, by definition, variable. > - const int SEG_CHUNK_SIZE = 256; > +#define SEG_CHUNK_SIZE 256 enum { SEG_CHUNK_SIZE = 256 }; would retain the function scope for the symbol, if that's desirable. r~
On 5/5/21 7:15 PM, Keith Busch wrote: > Thank you, that makes sense. In this case, we are talking about an > integer constant expression for the value of a 'const' symbol. That > seems like perfect fodder for a compiler to optimize. I understand it's > not obligated to do that, but I assumed it would. It probably did compile the code such that there is no variable-sized allocation from the stack. That optimization does not change the *semantics* of the code as written -- the only reason this compiles is via use of a variable length array. r~
diff --git a/hw/block/nvme.c b/hw/block/nvme.c index 5fe082ec34c..2f6d4925826 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -812,7 +812,7 @@ static uint16_t nvme_map_sgl(NvmeCtrl *n, NvmeSg *sg, NvmeSglDescriptor sgl, * descriptors and segment chain) than the command transfer size, so it is * not bounded by MDTS. */ - const int SEG_CHUNK_SIZE = 256; +#define SEG_CHUNK_SIZE 256 NvmeSglDescriptor segment[SEG_CHUNK_SIZE], *sgld, *last_sgld; uint64_t nsgld;
The compiler isn't clever enough to figure 'SEG_CHUNK_SIZE' is a constant! Help it by using a definitions instead. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/block/nvme.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)