Re: [Mesa-dev] [PATCH 1/8] nir: Silence missing field initializer warnings for nir_src
On Tue, 2015-12-15 at 22:44 -0800, Jason Ekstrand wrote: > On Dec 14, 2015 3:34 PM, "Ian Romanick" wrote: > > > > From: Ian Romanick > > > > nir/nir.h: In function 'nir_src_for_ssa': > > nir/nir.h:552:4: warning: missing initializer for field 'use_link' > of 'nir_src' > > [-Wmissing-field-initializers] > > nir_src src = NIR_SRC_INIT; > > ^ > > In file included from nir/nir.c:28:0: > > nir/nir.h:508:21: note: 'use_link' declared here > > struct list_head use_link; > > ^ > > > > Number of total warnings in my build reduced from 2329 to 1932 > > (reduction of 397). > Is there some reason why you're building with extra warnings? Last I > looked, all of NIR compiles with zero warnings with default flags so > the "removes compiler warnings" argument falls flat when the code is > perfectly well-defined. Just on this I build without adding extra warning flag and seem to be seeing issues that no-one else is, either that or people are ignoring warnings when pushing code and not fixing it after. I fixed 4b9a79b7b813b the other day after the warning seemed to have been around for a couple of weeks. I pulled this morning and now I have this in nir: brw_vue_map.c: In function 'brw_print_vue_map': brw_vue_map.c:260:20: warning: array subscript is above array bounds [ -Warray-bounds] return brw_names[slot - VARYING_SLOT_MAX]; And this in GLSL IR: lower_buffer_access.cpp: In member function 'bool lower_buffer_access::lower_buffer_access::is_dereferenced_thing_row_maj or(const ir_rvalue*)': lower_buffer_access.cpp:286:26: warning: unused variable 'var' [ -Wunused-variable] ir_variable *var = deref->variable_referenced(); I'm using gcc 5.3.1 and running Fedora if it makes any difference. > What utility does building with -wextra actually provide? > > If this serves no purpose other than alleviating your personal annoyance with > your preferred extra flags, then I'm going to have to nak it. I'm not being > territorial, I just don't see any point in adding complexity for the sake of > -wextra. > > --Jason > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org> http://lists.freedesktop.org/mailman/listinfo/mesa-dev> ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/8] nir: Silence missing field initializer warnings for nir_src
On Monday, December 14, 2015 03:34:25 PM Ian Romanick wrote: > From: Ian Romanick > > nir/nir.h: In function 'nir_src_for_ssa': > nir/nir.h:552:4: warning: missing initializer for field 'use_link' of > 'nir_src' > [-Wmissing-field-initializers] > nir_src src = NIR_SRC_INIT; > ^ > In file included from nir/nir.c:28:0: > nir/nir.h:508:21: note: 'use_link' declared here > struct list_head use_link; > ^ > > Number of total warnings in my build reduced from 2329 to 1932 > (reduction of 397). > > Signed-off-by: Ian Romanick > --- > src/glsl/nir/nir.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/glsl/nir/nir.h b/src/glsl/nir/nir.h > index 2e72e66..5543c52 100644 > --- a/src/glsl/nir/nir.h > +++ b/src/glsl/nir/nir.h > @@ -515,7 +515,7 @@ typedef struct nir_src { > bool is_ssa; > } nir_src; > > -#define NIR_SRC_INIT (nir_src) { { NULL } } > +#define NIR_SRC_INIT (nir_src) { { NULL }, { NULL, NULL }, { { NULL, NULL, 0 > } }, false } > > #define nir_foreach_use(reg_or_ssa_def, src) \ > list_for_each_entry(nir_src, src, &(reg_or_ssa_def)->uses, use_link) > Patches 1-2 are: Reviewed-by: Kenneth Graunke While I believe GCC does the right thing here, I believe that relying on {{NULL}} initializing all fields is technically relying on undefined behavior. It's good practice to initialize fields, and there's really no reason not to. True, it's annoying to have to maintain such a list when adding/changing fields in the structure - but I expect that changing nir_src will be pretty rare. It hasn't changed much since it was introduced. The right solution is to use C99 designated initializers - where unspecified fields are actually defined to be zero-initialized - but we can't due to MSVC 2013 bugs. Apparently this is fixed in MSVC 2015 [1], so when Jose lets us move to that, we can simplify this. [1] http://stackoverflow.com/questions/24090739/possible-compiler-bug-in-msvc12-vs2013-with-designated-initializer signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/8] nir: Silence missing field initializer warnings for nir_src
On Dec 14, 2015 3:34 PM, "Ian Romanick" wrote: > > From: Ian Romanick > > nir/nir.h: In function 'nir_src_for_ssa': > nir/nir.h:552:4: warning: missing initializer for field 'use_link' of 'nir_src' > [-Wmissing-field-initializers] > nir_src src = NIR_SRC_INIT; > ^ > In file included from nir/nir.c:28:0: > nir/nir.h:508:21: note: 'use_link' declared here > struct list_head use_link; > ^ > > Number of total warnings in my build reduced from 2329 to 1932 > (reduction of 397). Is there some reason why you're building with extra warnings? Last I looked, all of NIR compiles with zero warnings with default flags so the "removes compiler warnings" argument falls flat when the code is perfectly well-defined. What utility does building with -wextra actually provide? If this serves no purpose other than alleviating your personal annoyance with your preferred extra flags, then I'm going to have to nak it. I'm not being territorial, I just don't see any point in adding complexity for the sake of -wextra. --Jason ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/8] nir: Silence missing field initializer warnings for nir_src
On 12/14/2015 07:36 PM, Jason Ekstrand wrote: > On Mon, Dec 14, 2015 at 5:12 PM, Ian Romanick wrote: >> On 12/14/2015 04:39 PM, Ilia Mirkin wrote: >>> On Mon, Dec 14, 2015 at 7:28 PM, Ian Romanick wrote: On 12/14/2015 03:38 PM, Ilia Mirkin wrote: > It's a pretty standard feature of compilers to init things to 0 and > not have the full structure specified like that... what compiler are > you seeing these with? Can we just fix the glitch with a > -Wno-stupid-warnings? I have observed this with several versions of GCC. In C, you can avoid this with a trailing comma like: #define NIR_SRC_INIT (nir_src) { { NULL }, } However, nir.h is also used in some C++ code where that doesn't help. To be honest, I'm not a big fan of these macros. Without C99 designated initalizers, maintaining initializers like these (or the ones in src/glsl/builtin_variables.cpp) is a real pain. We can't use those, and we can't use C++ constructors. We have no good options available. :( I thought about replacing them with a static inline function that returns a zero-initialized struct. The compiler should generate the same code. However, that doesn't work with uses like those in patch 3. I'm also a little curious why you didn't raise this issue when I sent these patches out in August. I removed the patch from the series that you objected to back then. >>> >>> I have absolutely no recollection of any of that. Perhaps I saw "nir" >>> and thought to myself, "don't care, let them do whatever, this won't >>> ever affect me". Which is a sentiment I'm happy to continue with, by >>> the way. >> >> Fair enough. :) The patch I removed was one that removed the gl_context >> parameter from a function in dd_function_table. >> >> http://patchwork.freedesktop.org/patch/58048/ >> >>> I know that doing >>> >>> x = {} >>> >>> is a gcc extension, but I thought that {0} should always work (with >>> enough {} nesting in case the first element is a struct). Perhaps it >> >> {0} is, basically what we're doing now, and GCC complains about it with >> -Wmissing-field-initializers or -Wextra. When we added C-style struct > > I'm not a big fan of spending time fixing warnings that you have to > add -Wextra to get. However, if there are C++ issues, then those > definitely need to get fixed. Those options found real bugs in builtin_variables.cpp, and I'm a big fan of that. >> and array initializers to GLSL, we discussed adding this sort of >> implicit zero initialization. I did some digging in the C89 and C99 >> specs, and I have some recollection that in this case the missing fields >> get undefined values... but, starting with C99, {0, } implicitly >> initializes the missing fields to zero. I also seem to recall that bit >> of weirdness in C is why quite a few people were opposed to adding it to >> GLSL. This was several years ago, so my memory may not be completely >> reliable. >> >>> doesn't in C++? I could believe that, although I'd be surprised. >> >> The initializer support in C++ intentionally quite a bit more primitive >> than in C99. The language designers want you to use constructors >> whether it's the best tool for the job or not... which is why there are >> no designated initializers. > > So, I've got a patch somewhere that switches based on __cplusplus and > defines NIR_SRC_INIT as either the C99 thing or nir_src() for C++. I thought about doing something like that too. Having to maintain and keep in sync two separate versions of the initializer / constructor doesn't sound like a maintainable solution either. At best, it's the kind of thing that I expect someone to see in a year, say "WTF?", and submit a patch to change. At worst, in a year we decide to add some field to nir_src that isn't zero initialized, and we forget to update one of the initializers... and end up with a hard to find bug. > Would that solve this problem? There was also a bug recently about us > not building with oricle studio that it would probably fix. If so, > let's do that rather than a gigantic mess of braces and zeros. We explicitly removed support for Oracle Studio, so that's not a consideration. > --Jason > >>> Anyways, didn't mean to stir the pot too much, just thought there >>> might be a simpler way out of all this. >> >> Well, there are. :) We just can't use them due to some combination of >> MSVC, C++, and C99. >> >>> Cheers, >>> >>> -ilia >> >> ___ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/8] nir: Silence missing field initializer warnings for nir_src
On Mon, Dec 14, 2015 at 5:12 PM, Ian Romanick wrote: > On 12/14/2015 04:39 PM, Ilia Mirkin wrote: >> On Mon, Dec 14, 2015 at 7:28 PM, Ian Romanick wrote: >>> On 12/14/2015 03:38 PM, Ilia Mirkin wrote: It's a pretty standard feature of compilers to init things to 0 and not have the full structure specified like that... what compiler are you seeing these with? Can we just fix the glitch with a -Wno-stupid-warnings? >>> >>> I have observed this with several versions of GCC. >>> >>> In C, you can avoid this with a trailing comma like: >>> >>> #define NIR_SRC_INIT (nir_src) { { NULL }, } >>> >>> However, nir.h is also used in some C++ code where that doesn't help. >>> >>> To be honest, I'm not a big fan of these macros. Without C99 designated >>> initalizers, maintaining initializers like these (or the ones in >>> src/glsl/builtin_variables.cpp) is a real pain. We can't use those, and >>> we can't use C++ constructors. We have no good options available. :( >>> >>> I thought about replacing them with a static inline function that >>> returns a zero-initialized struct. The compiler should generate the >>> same code. However, that doesn't work with uses like those in patch 3. >>> >>> I'm also a little curious why you didn't raise this issue when I sent >>> these patches out in August. I removed the patch from the series that >>> you objected to back then. >> >> I have absolutely no recollection of any of that. Perhaps I saw "nir" >> and thought to myself, "don't care, let them do whatever, this won't >> ever affect me". Which is a sentiment I'm happy to continue with, by >> the way. > > Fair enough. :) The patch I removed was one that removed the gl_context > parameter from a function in dd_function_table. > > http://patchwork.freedesktop.org/patch/58048/ > >> I know that doing >> >> x = {} >> >> is a gcc extension, but I thought that {0} should always work (with >> enough {} nesting in case the first element is a struct). Perhaps it > > {0} is, basically what we're doing now, and GCC complains about it with > -Wmissing-field-initializers or -Wextra. When we added C-style struct I'm not a big fan of spending time fixing warnings that you have to add -Wextra to get. However, if there are C++ issues, then those definitely need to get fixed. > and array initializers to GLSL, we discussed adding this sort of > implicit zero initialization. I did some digging in the C89 and C99 > specs, and I have some recollection that in this case the missing fields > get undefined values... but, starting with C99, {0, } implicitly > initializes the missing fields to zero. I also seem to recall that bit > of weirdness in C is why quite a few people were opposed to adding it to > GLSL. This was several years ago, so my memory may not be completely > reliable. > >> doesn't in C++? I could believe that, although I'd be surprised. > > The initializer support in C++ intentionally quite a bit more primitive > than in C99. The language designers want you to use constructors > whether it's the best tool for the job or not... which is why there are > no designated initializers. So, I've got a patch somewhere that switches based on __cplusplus and defines NIR_SRC_INIT as either the C99 thing or nir_src() for C++. Would that solve this problem? There was also a bug recently about us not building with oricle studio that it would probably fix. If so, let's do that rather than a gigantic mess of braces and zeros. --Jason >> Anyways, didn't mean to stir the pot too much, just thought there >> might be a simpler way out of all this. > > Well, there are. :) We just can't use them due to some combination of > MSVC, C++, and C99. > >> Cheers, >> >> -ilia > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/8] nir: Silence missing field initializer warnings for nir_src
On 12/14/2015 04:39 PM, Ilia Mirkin wrote: > On Mon, Dec 14, 2015 at 7:28 PM, Ian Romanick wrote: >> On 12/14/2015 03:38 PM, Ilia Mirkin wrote: >>> It's a pretty standard feature of compilers to init things to 0 and >>> not have the full structure specified like that... what compiler are >>> you seeing these with? Can we just fix the glitch with a >>> -Wno-stupid-warnings? >> >> I have observed this with several versions of GCC. >> >> In C, you can avoid this with a trailing comma like: >> >> #define NIR_SRC_INIT (nir_src) { { NULL }, } >> >> However, nir.h is also used in some C++ code where that doesn't help. >> >> To be honest, I'm not a big fan of these macros. Without C99 designated >> initalizers, maintaining initializers like these (or the ones in >> src/glsl/builtin_variables.cpp) is a real pain. We can't use those, and >> we can't use C++ constructors. We have no good options available. :( >> >> I thought about replacing them with a static inline function that >> returns a zero-initialized struct. The compiler should generate the >> same code. However, that doesn't work with uses like those in patch 3. >> >> I'm also a little curious why you didn't raise this issue when I sent >> these patches out in August. I removed the patch from the series that >> you objected to back then. > > I have absolutely no recollection of any of that. Perhaps I saw "nir" > and thought to myself, "don't care, let them do whatever, this won't > ever affect me". Which is a sentiment I'm happy to continue with, by > the way. Fair enough. :) The patch I removed was one that removed the gl_context parameter from a function in dd_function_table. http://patchwork.freedesktop.org/patch/58048/ > I know that doing > > x = {} > > is a gcc extension, but I thought that {0} should always work (with > enough {} nesting in case the first element is a struct). Perhaps it {0} is, basically what we're doing now, and GCC complains about it with -Wmissing-field-initializers or -Wextra. When we added C-style struct and array initializers to GLSL, we discussed adding this sort of implicit zero initialization. I did some digging in the C89 and C99 specs, and I have some recollection that in this case the missing fields get undefined values... but, starting with C99, {0, } implicitly initializes the missing fields to zero. I also seem to recall that bit of weirdness in C is why quite a few people were opposed to adding it to GLSL. This was several years ago, so my memory may not be completely reliable. > doesn't in C++? I could believe that, although I'd be surprised. The initializer support in C++ intentionally quite a bit more primitive than in C99. The language designers want you to use constructors whether it's the best tool for the job or not... which is why there are no designated initializers. > Anyways, didn't mean to stir the pot too much, just thought there > might be a simpler way out of all this. Well, there are. :) We just can't use them due to some combination of MSVC, C++, and C99. > Cheers, > > -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/8] nir: Silence missing field initializer warnings for nir_src
On Mon, Dec 14, 2015 at 7:28 PM, Ian Romanick wrote: > On 12/14/2015 03:38 PM, Ilia Mirkin wrote: >> It's a pretty standard feature of compilers to init things to 0 and >> not have the full structure specified like that... what compiler are >> you seeing these with? Can we just fix the glitch with a >> -Wno-stupid-warnings? > > I have observed this with several versions of GCC. > > In C, you can avoid this with a trailing comma like: > > #define NIR_SRC_INIT (nir_src) { { NULL }, } > > However, nir.h is also used in some C++ code where that doesn't help. > > To be honest, I'm not a big fan of these macros. Without C99 designated > initalizers, maintaining initializers like these (or the ones in > src/glsl/builtin_variables.cpp) is a real pain. We can't use those, and > we can't use C++ constructors. We have no good options available. :( > > I thought about replacing them with a static inline function that > returns a zero-initialized struct. The compiler should generate the > same code. However, that doesn't work with uses like those in patch 3. > > I'm also a little curious why you didn't raise this issue when I sent > these patches out in August. I removed the patch from the series that > you objected to back then. I have absolutely no recollection of any of that. Perhaps I saw "nir" and thought to myself, "don't care, let them do whatever, this won't ever affect me". Which is a sentiment I'm happy to continue with, by the way. I know that doing x = {} is a gcc extension, but I thought that {0} should always work (with enough {} nesting in case the first element is a struct). Perhaps it doesn't in C++? I could believe that, although I'd be surprised. Anyways, didn't mean to stir the pot too much, just thought there might be a simpler way out of all this. Cheers, -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/8] nir: Silence missing field initializer warnings for nir_src
On 12/14/2015 03:38 PM, Ilia Mirkin wrote: > It's a pretty standard feature of compilers to init things to 0 and > not have the full structure specified like that... what compiler are > you seeing these with? Can we just fix the glitch with a > -Wno-stupid-warnings? I have observed this with several versions of GCC. In C, you can avoid this with a trailing comma like: #define NIR_SRC_INIT (nir_src) { { NULL }, } However, nir.h is also used in some C++ code where that doesn't help. To be honest, I'm not a big fan of these macros. Without C99 designated initalizers, maintaining initializers like these (or the ones in src/glsl/builtin_variables.cpp) is a real pain. We can't use those, and we can't use C++ constructors. We have no good options available. :( I thought about replacing them with a static inline function that returns a zero-initialized struct. The compiler should generate the same code. However, that doesn't work with uses like those in patch 3. I'm also a little curious why you didn't raise this issue when I sent these patches out in August. I removed the patch from the series that you objected to back then. > On Mon, Dec 14, 2015 at 6:34 PM, Ian Romanick wrote: >> From: Ian Romanick >> >> nir/nir.h: In function 'nir_src_for_ssa': >> nir/nir.h:552:4: warning: missing initializer for field 'use_link' of >> 'nir_src' >> [-Wmissing-field-initializers] >> nir_src src = NIR_SRC_INIT; >> ^ >> In file included from nir/nir.c:28:0: >> nir/nir.h:508:21: note: 'use_link' declared here >> struct list_head use_link; >> ^ >> >> Number of total warnings in my build reduced from 2329 to 1932 >> (reduction of 397). >> >> Signed-off-by: Ian Romanick >> --- >> src/glsl/nir/nir.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/src/glsl/nir/nir.h b/src/glsl/nir/nir.h >> index 2e72e66..5543c52 100644 >> --- a/src/glsl/nir/nir.h >> +++ b/src/glsl/nir/nir.h >> @@ -515,7 +515,7 @@ typedef struct nir_src { >> bool is_ssa; >> } nir_src; >> >> -#define NIR_SRC_INIT (nir_src) { { NULL } } >> +#define NIR_SRC_INIT (nir_src) { { NULL }, { NULL, NULL }, { { NULL, NULL, >> 0 } }, false } >> >> #define nir_foreach_use(reg_or_ssa_def, src) \ >> list_for_each_entry(nir_src, src, &(reg_or_ssa_def)->uses, use_link) >> -- >> 2.5.0 >> >> ___ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/8] nir: Silence missing field initializer warnings for nir_src
It's a pretty standard feature of compilers to init things to 0 and not have the full structure specified like that... what compiler are you seeing these with? Can we just fix the glitch with a -Wno-stupid-warnings? On Mon, Dec 14, 2015 at 6:34 PM, Ian Romanick wrote: > From: Ian Romanick > > nir/nir.h: In function 'nir_src_for_ssa': > nir/nir.h:552:4: warning: missing initializer for field 'use_link' of > 'nir_src' > [-Wmissing-field-initializers] > nir_src src = NIR_SRC_INIT; > ^ > In file included from nir/nir.c:28:0: > nir/nir.h:508:21: note: 'use_link' declared here > struct list_head use_link; > ^ > > Number of total warnings in my build reduced from 2329 to 1932 > (reduction of 397). > > Signed-off-by: Ian Romanick > --- > src/glsl/nir/nir.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/glsl/nir/nir.h b/src/glsl/nir/nir.h > index 2e72e66..5543c52 100644 > --- a/src/glsl/nir/nir.h > +++ b/src/glsl/nir/nir.h > @@ -515,7 +515,7 @@ typedef struct nir_src { > bool is_ssa; > } nir_src; > > -#define NIR_SRC_INIT (nir_src) { { NULL } } > +#define NIR_SRC_INIT (nir_src) { { NULL }, { NULL, NULL }, { { NULL, NULL, 0 > } }, false } > > #define nir_foreach_use(reg_or_ssa_def, src) \ > list_for_each_entry(nir_src, src, &(reg_or_ssa_def)->uses, use_link) > -- > 2.5.0 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev