Re: [Mesa-dev] [PATCH] st/mesa: init hash keys with memset(), not designated initializers
Am 12.03.19 um 18:34 schrieb Eric Anholt:
> Ilia Mirkin writes:
>
>> I believe the distinction here is between initializing all the fields
>> and using them individually (thus leaving the padding uninitialized),
>> vs using the fields to create a structure to hash as a sequence of
>> bytes. For the latter, you really need all the memory initialized, not
>> just the "valid" parts of the structure. In at least my mind, it's
>> fairly well-established that compilers don't always initialize all of
>> a structure's underlying bytes, but I also don't have a specific
>> instance of that situation I can point to.
>>
>> For most usage, foo = {0} is fine since you're not hashing the bytes
>> but rather accessing the fields directly. But for hashing, you really
>> want all the bits initialized.
>
> Gah. The commit message even said it was about padding, and I failed to
> read. Sorry for the noise, this does seem right.
The alternative is to use explicit padding in the struct, so that
structure initialization is guaranteed to initialize everything to zero.
I believe some places still do that too, but it's not very nice neither
(can easily make mistakes there). So - pick your poison...
Roland
>
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fmesa-devdata=02%7C01%7Csroland%40vmware.com%7Cd6e706866d844beb84a808d6a7110f53%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636880089068822714sdata=AENcqpYCV4d4DJW3q4Bkg9GXIEyU6Au8Yccuthd7Mbk%3Dreserved=0
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] st/mesa: init hash keys with memset(), not designated initializers
Ilia Mirkin writes:
> I believe the distinction here is between initializing all the fields
> and using them individually (thus leaving the padding uninitialized),
> vs using the fields to create a structure to hash as a sequence of
> bytes. For the latter, you really need all the memory initialized, not
> just the "valid" parts of the structure. In at least my mind, it's
> fairly well-established that compilers don't always initialize all of
> a structure's underlying bytes, but I also don't have a specific
> instance of that situation I can point to.
>
> For most usage, foo = {0} is fine since you're not hashing the bytes
> but rather accessing the fields directly. But for hashing, you really
> want all the bits initialized.
Gah. The commit message even said it was about padding, and I failed to
read. Sorry for the noise, this does seem right.
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] st/mesa: init hash keys with memset(), not designated initializers
I believe the distinction here is between initializing all the fields
and using them individually (thus leaving the padding uninitialized),
vs using the fields to create a structure to hash as a sequence of
bytes. For the latter, you really need all the memory initialized, not
just the "valid" parts of the structure. In at least my mind, it's
fairly well-established that compilers don't always initialize all of
a structure's underlying bytes, but I also don't have a specific
instance of that situation I can point to.
For most usage, foo = {0} is fine since you're not hashing the bytes
but rather accessing the fields directly. But for hashing, you really
want all the bits initialized.
Cheers,
-ilia
On Tue, Mar 12, 2019 at 12:31 PM Eric Anholt wrote:
>
> Brian Paul writes:
>
> > On 03/11/2019 04:47 PM, Eric Anholt wrote:
> >> Brian Paul writes:
> >>
> >>> Since the compiler may not zero-out padding in the object.
> >>> Add a couple comments about this to prevent misunderstandings in
> >>> the future.
> >>>
> >>> Fixes: 67d96816ff5 ("st/mesa: move, clean-up shader variant key
> >>> decls/inits")
> >>> ---
> >>> src/mesa/state_tracker/st_atom_shader.c | 9 +++--
> >>> src/mesa/state_tracker/st_program.c | 13 ++---
> >>> 2 files changed, 17 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/src/mesa/state_tracker/st_atom_shader.c
> >>> b/src/mesa/state_tracker/st_atom_shader.c
> >>> index ac7a1a5..a4475e2 100644
> >>> --- a/src/mesa/state_tracker/st_atom_shader.c
> >>> +++ b/src/mesa/state_tracker/st_atom_shader.c
> >>> @@ -112,7 +112,10 @@ st_update_fp( struct st_context *st )
> >>> !stfp->variants->key.bitmap) {
> >>> shader = stfp->variants->driver_shader;
> >>> } else {
> >>> - struct st_fp_variant_key key = {0};
> >>> + struct st_fp_variant_key key;
> >>> +
> >>> + /* use memset, not an initializer to be sure all memory is zeroed
> >>> */
> >>> + memset(, 0, sizeof(key));
> >>
> >> Wait, what? We rely on this form of initialization all over, what's
> >> changed?
> >
> > The question is do all compilers, when presented with
> >
> > struct st_fp_variant_key key = {0};
>
> You seem to be saying that not all compilers do, but throughout the
> compiler we're relying on all compilers doing so. Can we get some more
> information about what compiler you're fixing here?
> -BEGIN PGP SIGNATURE-
>
> iQIzBAEBCgAdFiEE/JuuFDWp9/ZkuCBXtdYpNtH8nugFAlyH3tsACgkQtdYpNtH8
> nujFFxAAkzNBlIJyvZhaUuYZ3B7ugbHxCnyfsi6J9AmAKcXyyJ0oIAi1sXkLwflB
> e5WbyksenHiE+NnihdQZICV2fzb0G8WObn5HiBSxNGKqEwATQY3ND3gkzvH2npKT
> EGY0AZCPX/yJPA3yci/MRTLLPzLgSYTUstfEsAmTlRlqmOlWDenV6BV7OpOoTz/F
> bVdrk6QXufLcQ+ZtQS2OFKEKlXdaUaE5ruwanasc8qzXmJ0TMonvRZp4ZrNUNc4j
> p1sUQnAU0SxwuSqIVS6iVmPEH/N2e3u4XdNsZ+45KxFkAnV0wnpIKAbREa1tzOdP
> d2nhZ70BUNnHJE5q11I070mGWkburTKk5hbLvFGS59np/2nLjZQLcb3hYNyCPDP2
> sdRwqzPtwbdNLAbHpiQ3x0OuVJ6P5fuxGUjuB/AQfl1ixV03nB77AXogqhlQ/cK4
> WceUn9AXmhBb5tIdhfKK1uzuplmCSpXaKxdPubvgfI4Rey5KMvcZwBFNqztcfe24
> hc3wPFyfmayNAuFHkC+0jd4po20ibJkF1F0kXjoyl7dKMG6IFX4fICJKPsafIgNV
> cex67m6VAF9ZalBaSgnXZpcmHXSFbVSHYvES/WxRncefgAzXG/BMuywsLZ+US99A
> LfrnLaq0eRDAdpcSfITQmDgpjEvWFThxCDXXBCI5a+pQpYBB6lk=
> =4Bvh
> -END PGP SIGNATURE-___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] st/mesa: init hash keys with memset(), not designated initializers
Brian Paul writes:
> On 03/11/2019 04:47 PM, Eric Anholt wrote:
>> Brian Paul writes:
>>
>>> Since the compiler may not zero-out padding in the object.
>>> Add a couple comments about this to prevent misunderstandings in
>>> the future.
>>>
>>> Fixes: 67d96816ff5 ("st/mesa: move, clean-up shader variant key
>>> decls/inits")
>>> ---
>>> src/mesa/state_tracker/st_atom_shader.c | 9 +++--
>>> src/mesa/state_tracker/st_program.c | 13 ++---
>>> 2 files changed, 17 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/mesa/state_tracker/st_atom_shader.c
>>> b/src/mesa/state_tracker/st_atom_shader.c
>>> index ac7a1a5..a4475e2 100644
>>> --- a/src/mesa/state_tracker/st_atom_shader.c
>>> +++ b/src/mesa/state_tracker/st_atom_shader.c
>>> @@ -112,7 +112,10 @@ st_update_fp( struct st_context *st )
>>> !stfp->variants->key.bitmap) {
>>> shader = stfp->variants->driver_shader;
>>> } else {
>>> - struct st_fp_variant_key key = {0};
>>> + struct st_fp_variant_key key;
>>> +
>>> + /* use memset, not an initializer to be sure all memory is zeroed */
>>> + memset(, 0, sizeof(key));
>>
>> Wait, what? We rely on this form of initialization all over, what's
>> changed?
>
> The question is do all compilers, when presented with
>
> struct st_fp_variant_key key = {0};
You seem to be saying that not all compilers do, but throughout the
compiler we're relying on all compilers doing so. Can we get some more
information about what compiler you're fixing here?
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] st/mesa: init hash keys with memset(), not designated initializers
On 03/11/2019 04:47 PM, Eric Anholt wrote:
Brian Paul writes:
Since the compiler may not zero-out padding in the object.
Add a couple comments about this to prevent misunderstandings in
the future.
Fixes: 67d96816ff5 ("st/mesa: move, clean-up shader variant key decls/inits")
---
src/mesa/state_tracker/st_atom_shader.c | 9 +++--
src/mesa/state_tracker/st_program.c | 13 ++---
2 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/src/mesa/state_tracker/st_atom_shader.c
b/src/mesa/state_tracker/st_atom_shader.c
index ac7a1a5..a4475e2 100644
--- a/src/mesa/state_tracker/st_atom_shader.c
+++ b/src/mesa/state_tracker/st_atom_shader.c
@@ -112,7 +112,10 @@ st_update_fp( struct st_context *st )
!stfp->variants->key.bitmap) {
shader = stfp->variants->driver_shader;
} else {
- struct st_fp_variant_key key = {0};
+ struct st_fp_variant_key key;
+
+ /* use memset, not an initializer to be sure all memory is zeroed */
+ memset(, 0, sizeof(key));
Wait, what? We rely on this form of initialization all over, what's
changed?
The question is do all compilers, when presented with
struct st_fp_variant_key key = {0};
initialize the entire object to zero, or just the individual fields
(skipping padding).
This matters for hash keys but shouldn't matter otherwise.
-Brian
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] st/mesa: init hash keys with memset(), not designated initializers
Brian Paul writes:
> Since the compiler may not zero-out padding in the object.
> Add a couple comments about this to prevent misunderstandings in
> the future.
>
> Fixes: 67d96816ff5 ("st/mesa: move, clean-up shader variant key decls/inits")
> ---
> src/mesa/state_tracker/st_atom_shader.c | 9 +++--
> src/mesa/state_tracker/st_program.c | 13 ++---
> 2 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/src/mesa/state_tracker/st_atom_shader.c
> b/src/mesa/state_tracker/st_atom_shader.c
> index ac7a1a5..a4475e2 100644
> --- a/src/mesa/state_tracker/st_atom_shader.c
> +++ b/src/mesa/state_tracker/st_atom_shader.c
> @@ -112,7 +112,10 @@ st_update_fp( struct st_context *st )
> !stfp->variants->key.bitmap) {
>shader = stfp->variants->driver_shader;
> } else {
> - struct st_fp_variant_key key = {0};
> + struct st_fp_variant_key key;
> +
> + /* use memset, not an initializer to be sure all memory is zeroed */
> + memset(, 0, sizeof(key));
Wait, what? We rely on this form of initialization all over, what's
changed?
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] st/mesa: init hash keys with memset(), not designated initializers
Reviewed-by: Neha Bhende
Regards,
Neha
From: mesa-dev on behalf of Brian Paul
Sent: Friday, March 8, 2019 9:14 AM
To: mesa-dev@lists.freedesktop.org
Cc: Neha Bhende
Subject: [Mesa-dev] [PATCH] st/mesa: init hash keys with memset(), not
designated initializers
Since the compiler may not zero-out padding in the object.
Add a couple comments about this to prevent misunderstandings in
the future.
Fixes: 67d96816ff5 ("st/mesa: move, clean-up shader variant key decls/inits")
---
src/mesa/state_tracker/st_atom_shader.c | 9 +++--
src/mesa/state_tracker/st_program.c | 13 ++---
2 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/src/mesa/state_tracker/st_atom_shader.c
b/src/mesa/state_tracker/st_atom_shader.c
index ac7a1a5..a4475e2 100644
--- a/src/mesa/state_tracker/st_atom_shader.c
+++ b/src/mesa/state_tracker/st_atom_shader.c
@@ -112,7 +112,10 @@ st_update_fp( struct st_context *st )
!stfp->variants->key.bitmap) {
shader = stfp->variants->driver_shader;
} else {
- struct st_fp_variant_key key = {0};
+ struct st_fp_variant_key key;
+
+ /* use memset, not an initializer to be sure all memory is zeroed */
+ memset(, 0, sizeof(key));
key.st = st->has_shareable_shaders ? NULL : st;
@@ -168,7 +171,9 @@ st_update_vp( struct st_context *st )
stvp->variants->key.passthrough_edgeflags == st->vertdata_edgeflags) {
st->vp_variant = stvp->variants;
} else {
- struct st_vp_variant_key key = {0};
+ struct st_vp_variant_key key;
+
+ memset(, 0, sizeof(key));
key.st = st->has_shareable_shaders ? NULL : st;
diff --git a/src/mesa/state_tracker/st_program.c
b/src/mesa/state_tracker/st_program.c
index 6d669a9..fe03070 100644
--- a/src/mesa/state_tracker/st_program.c
+++ b/src/mesa/state_tracker/st_program.c
@@ -1807,7 +1807,10 @@ st_get_cp_variant(struct st_context *st,
{
struct pipe_context *pipe = st->pipe;
struct st_basic_variant *v;
- struct st_basic_variant_key key = {0};
+ struct st_basic_variant_key key;
+
+ /* use memset, not an initializer to be sure all memory is zeroed */
+ memset(, 0, sizeof(key));
key.st = st->has_shareable_shaders ? NULL : st;
@@ -2030,7 +2033,9 @@ st_precompile_shader_variant(struct st_context *st,
switch (prog->Target) {
case GL_VERTEX_PROGRAM_ARB: {
struct st_vertex_program *p = (struct st_vertex_program *)prog;
- struct st_vp_variant_key key = {0};
+ struct st_vp_variant_key key;
+
+ memset(, 0, sizeof(key));
key.st = st->has_shareable_shaders ? NULL : st;
st_get_vp_variant(st, p, );
@@ -2057,7 +2062,9 @@ st_precompile_shader_variant(struct st_context *st,
case GL_FRAGMENT_PROGRAM_ARB: {
struct st_fragment_program *p = (struct st_fragment_program *)prog;
- struct st_fp_variant_key key = {0};
+ struct st_fp_variant_key key;
+
+ memset(, 0, sizeof(key));
key.st = st->has_shareable_shaders ? NULL : st;
st_get_fp_variant(st, p, );
--
1.8.5.6
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fmesa-devdata=02%7C01%7Cbhenden%40vmware.com%7C773d52000a58466ac0d508d6a3e98df6%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636876620836813700sdata=pyvSVyPLzF8JXaiMptIRwvddQX2Hh7rm%2FAV%2BivL%2FOOE%3Dreserved=0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] st/mesa: init hash keys with memset(), not designated initializers
Reviewed-by: Roland Scheidegger
Am 08.03.19 um 18:14 schrieb Brian Paul:
> Since the compiler may not zero-out padding in the object.
> Add a couple comments about this to prevent misunderstandings in
> the future.
>
> Fixes: 67d96816ff5 ("st/mesa: move, clean-up shader variant key decls/inits")
> ---
> src/mesa/state_tracker/st_atom_shader.c | 9 +++--
> src/mesa/state_tracker/st_program.c | 13 ++---
> 2 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/src/mesa/state_tracker/st_atom_shader.c
> b/src/mesa/state_tracker/st_atom_shader.c
> index ac7a1a5..a4475e2 100644
> --- a/src/mesa/state_tracker/st_atom_shader.c
> +++ b/src/mesa/state_tracker/st_atom_shader.c
> @@ -112,7 +112,10 @@ st_update_fp( struct st_context *st )
> !stfp->variants->key.bitmap) {
>shader = stfp->variants->driver_shader;
> } else {
> - struct st_fp_variant_key key = {0};
> + struct st_fp_variant_key key;
> +
> + /* use memset, not an initializer to be sure all memory is zeroed */
> + memset(, 0, sizeof(key));
>
>key.st = st->has_shareable_shaders ? NULL : st;
>
> @@ -168,7 +171,9 @@ st_update_vp( struct st_context *st )
> stvp->variants->key.passthrough_edgeflags == st->vertdata_edgeflags) {
>st->vp_variant = stvp->variants;
> } else {
> - struct st_vp_variant_key key = {0};
> + struct st_vp_variant_key key;
> +
> + memset(, 0, sizeof(key));
>
>key.st = st->has_shareable_shaders ? NULL : st;
>
> diff --git a/src/mesa/state_tracker/st_program.c
> b/src/mesa/state_tracker/st_program.c
> index 6d669a9..fe03070 100644
> --- a/src/mesa/state_tracker/st_program.c
> +++ b/src/mesa/state_tracker/st_program.c
> @@ -1807,7 +1807,10 @@ st_get_cp_variant(struct st_context *st,
> {
> struct pipe_context *pipe = st->pipe;
> struct st_basic_variant *v;
> - struct st_basic_variant_key key = {0};
> + struct st_basic_variant_key key;
> +
> + /* use memset, not an initializer to be sure all memory is zeroed */
> + memset(, 0, sizeof(key));
>
> key.st = st->has_shareable_shaders ? NULL : st;
>
> @@ -2030,7 +2033,9 @@ st_precompile_shader_variant(struct st_context *st,
> switch (prog->Target) {
> case GL_VERTEX_PROGRAM_ARB: {
>struct st_vertex_program *p = (struct st_vertex_program *)prog;
> - struct st_vp_variant_key key = {0};
> + struct st_vp_variant_key key;
> +
> + memset(, 0, sizeof(key));
>
>key.st = st->has_shareable_shaders ? NULL : st;
>st_get_vp_variant(st, p, );
> @@ -2057,7 +2062,9 @@ st_precompile_shader_variant(struct st_context *st,
>
> case GL_FRAGMENT_PROGRAM_ARB: {
>struct st_fragment_program *p = (struct st_fragment_program *)prog;
> - struct st_fp_variant_key key = {0};
> + struct st_fp_variant_key key;
> +
> + memset(, 0, sizeof(key));
>
>key.st = st->has_shareable_shaders ? NULL : st;
>st_get_fp_variant(st, p, );
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] st/mesa: init hash keys with memset(), not designated initializers
Since the compiler may not zero-out padding in the object.
Add a couple comments about this to prevent misunderstandings in
the future.
Fixes: 67d96816ff5 ("st/mesa: move, clean-up shader variant key decls/inits")
---
src/mesa/state_tracker/st_atom_shader.c | 9 +++--
src/mesa/state_tracker/st_program.c | 13 ++---
2 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/src/mesa/state_tracker/st_atom_shader.c
b/src/mesa/state_tracker/st_atom_shader.c
index ac7a1a5..a4475e2 100644
--- a/src/mesa/state_tracker/st_atom_shader.c
+++ b/src/mesa/state_tracker/st_atom_shader.c
@@ -112,7 +112,10 @@ st_update_fp( struct st_context *st )
!stfp->variants->key.bitmap) {
shader = stfp->variants->driver_shader;
} else {
- struct st_fp_variant_key key = {0};
+ struct st_fp_variant_key key;
+
+ /* use memset, not an initializer to be sure all memory is zeroed */
+ memset(, 0, sizeof(key));
key.st = st->has_shareable_shaders ? NULL : st;
@@ -168,7 +171,9 @@ st_update_vp( struct st_context *st )
stvp->variants->key.passthrough_edgeflags == st->vertdata_edgeflags) {
st->vp_variant = stvp->variants;
} else {
- struct st_vp_variant_key key = {0};
+ struct st_vp_variant_key key;
+
+ memset(, 0, sizeof(key));
key.st = st->has_shareable_shaders ? NULL : st;
diff --git a/src/mesa/state_tracker/st_program.c
b/src/mesa/state_tracker/st_program.c
index 6d669a9..fe03070 100644
--- a/src/mesa/state_tracker/st_program.c
+++ b/src/mesa/state_tracker/st_program.c
@@ -1807,7 +1807,10 @@ st_get_cp_variant(struct st_context *st,
{
struct pipe_context *pipe = st->pipe;
struct st_basic_variant *v;
- struct st_basic_variant_key key = {0};
+ struct st_basic_variant_key key;
+
+ /* use memset, not an initializer to be sure all memory is zeroed */
+ memset(, 0, sizeof(key));
key.st = st->has_shareable_shaders ? NULL : st;
@@ -2030,7 +2033,9 @@ st_precompile_shader_variant(struct st_context *st,
switch (prog->Target) {
case GL_VERTEX_PROGRAM_ARB: {
struct st_vertex_program *p = (struct st_vertex_program *)prog;
- struct st_vp_variant_key key = {0};
+ struct st_vp_variant_key key;
+
+ memset(, 0, sizeof(key));
key.st = st->has_shareable_shaders ? NULL : st;
st_get_vp_variant(st, p, );
@@ -2057,7 +2062,9 @@ st_precompile_shader_variant(struct st_context *st,
case GL_FRAGMENT_PROGRAM_ARB: {
struct st_fragment_program *p = (struct st_fragment_program *)prog;
- struct st_fp_variant_key key = {0};
+ struct st_fp_variant_key key;
+
+ memset(, 0, sizeof(key));
key.st = st->has_shareable_shaders ? NULL : st;
st_get_fp_variant(st, p, );
--
1.8.5.6
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
