Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.
On 6/9/21 4:47 AM, Robin Dapp wrote: As you say, the logic is convoluted. Let's simplify it rather than make it more convoluted. One possibility would be to change || to | toavoid the shortcut, and then bool note = lastalign > curalign; if (note) curalign = lastalign; I went with your suggestion in the attached v2. Regtested and bootstrapped on s390x, x86 and ppc64le. OK. urg, I did not commit yet because I fiddled around with my tests again. Using dg-notes checks (and thus being "forced" to handle all cases individually) I realized that the above is not enough as it reintroduces the same problem that I was originally trying to avoid: We want to warn if lastalign == curalign but DECL_USER_ALIGN (last_decl) != DECL_USER_ALIGN (decl), i.e. when the user explicitly specified __attribute__ ((aligned (8))) on s390 (see example in my first mail). What about checking lastalign > curalign || (lastalign == curalign && DECL_USER_ALIGN (last_decl) != DECL_USER_ALIGN (decl)) instead and changing the associated comment? I might use > instead of !=, but sure. Even then it's not really satisfactory to emit a note that complains about "aligned (8)" at a declaration where it was not even explicitly specified. But then, the situation is similar for other attributes as well and we would need to explicitly store the location where an attribute was specified first.
Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.
As you say, the logic is convoluted. Let's simplify it rather than make it more convoluted. One possibility would be to change || to | to avoid the shortcut, and then bool note = lastalign > curalign; if (note) curalign = lastalign; I went with your suggestion in the attached v2. Regtested and bootstrapped on s390x, x86 and ppc64le. OK. urg, I did not commit yet because I fiddled around with my tests again. Using dg-notes checks (and thus being "forced" to handle all cases individually) I realized that the above is not enough as it reintroduces the same problem that I was originally trying to avoid: We want to warn if lastalign == curalign but DECL_USER_ALIGN (last_decl) != DECL_USER_ALIGN (decl), i.e. when the user explicitly specified __attribute__ ((aligned (8))) on s390 (see example in my first mail). What about checking lastalign > curalign || (lastalign == curalign && DECL_USER_ALIGN (last_decl) != DECL_USER_ALIGN (decl)) instead and changing the associated comment? Even then it's not really satisfactory to emit a note that complains about "aligned (8)" at a declaration where it was not even explicitly specified. But then, the situation is similar for other attributes as well and we would need to explicitly store the location where an attribute was specified first. Regards Robin
Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.
On 6/1/21 9:20 AM, Robin Dapp wrote: As you say, the logic is convoluted. Let's simplify it rather than make it more convoluted. One possibility would be to change || to | to avoid the shortcut, and then bool note = lastalign > curalign; if (note) curalign = lastalign; I went with your suggestion in the attached v2. Regtested and bootstrapped on s390x, x86 and ppc64le. OK. Jason
Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.
This is the revised testsuite change with v2 adding a check for no separate note for __attribute__((aligned (32), aligned (4)). Regards Robin diff --git a/gcc/testsuite/c-c++-common/Wattributes.c b/gcc/testsuite/c-c++-common/Wattributes.c index 4ad90441b4d..a97e5ad5f74 100644 --- a/gcc/testsuite/c-c++-common/Wattributes.c +++ b/gcc/testsuite/c-c++-common/Wattributes.c @@ -97,6 +97,8 @@ fnoinline1 (void); /* { dg-message "previous declaration here" } */ /* Verify a warning for always_inline conflict. */ void ATTR ((always_inline)) fnoinline1 (void) { }/* { dg-warning "ignoring attribute .always_inline. because it conflicts with attribute .noinline." } */ + /* { dg-message "note: previous declaration here" "" { target *-*-* } .-1 } */ + /* { dg-message "note: previous definition" "" { target *-*-* } .-2 } */ /* Verify a warning for gnu_inline conflict. */ inline void ATTR ((gnu_inline)) @@ -364,13 +366,15 @@ inline int ATTR ((cold)) finline_cold_noreturn (int); inline int ATTR ((noreturn)) -finline_cold_noreturn (int); +finline_cold_noreturn (int); /* { dg-message "note: previous declaration here" } */ inline int ATTR ((noinline)) finline_cold_noreturn (int);/* { dg-warning "ignoring attribute .noinline. because it conflicts with attribute .always_inline." } */ +/* { dg-message "note: previous declaration here" "" { target *-*-* } .-1 } */ inline int ATTR ((hot)) finline_cold_noreturn (int);/* { dg-warning "ignoring attribute .hot. because it conflicts with attribute .cold." } */ +/* { dg-message "note: previous declaration here" "" { target *-*-* } .-1 } */ inline int ATTR ((warn_unused_result)) finline_cold_noreturn (int);/* { dg-warning "ignoring attribute .warn_unused_result. because it conflicts with attribute .noreturn." } */ @@ -389,23 +393,24 @@ finline_cold_noreturn (int i) { (void) __builtin_abort (); } and some on distinct declarations. */ inline int ATTR ((always_inline, hot)) -finline_hot_noret_align (int); +finline_hot_noret_align (int); /* { dg-message "note: previous declaration here" } */ inline int ATTR ((noreturn, noinline)) finline_hot_noret_align (int); /* { dg-warning "ignoring attribute .noinline. because it conflicts with attribute .always_inline." } */ +/* { dg-message "note: previous declaration here" "" { target *-*-* } .-1 } */ inline int ATTR ((cold, aligned (8))) finline_hot_noret_align (int); /* { dg-warning "ignoring attribute .cold. because it conflicts with attribute .hot." } */ +/* { dg-message "note: previous declaration here" "" { target *-*-* } .-1 } */ inline int ATTR ((warn_unused_result)) finline_hot_noret_align (int); /* { dg-warning "ignoring attribute .warn_unused_result. because it conflicts with attribute .noreturn." } */ inline int ATTR ((aligned (4))) - finline_hot_noret_align (int); /* { dg-warning "ignoring attribute .aligned \\(4\\). because it conflicts with attribute .aligned \\(8\\)." "" { target { ! { hppa*64*-*-* s390*-*-* } } } } */ -/* { dg-error "alignment for '.*finline_hot_noret_align.*' must be at least 8" "" { target s390*-*-* } .-1 } */ + finline_hot_noret_align (int); /* { dg-warning "ignoring attribute .aligned \\(4\\). because it conflicts with attribute .aligned \\(8\\)." "" { target { ! { hppa*64*-*-* } } } } */ inline int ATTR ((aligned (8))) -finline_hot_noret_align (int); +finline_hot_noret_align (int); /* { dg-message "note: previous declaration here" } */ inline int ATTR ((const)) finline_hot_noret_align (int); /* { dg-warning "ignoring attribute .const. because it conflicts with attribute .noreturn." } */ @@ -416,6 +421,26 @@ inline int ATTR ((noreturn)) finline_hot_noret_align (int i) { (void) __builtin_abort (); } +/* Expect a warning about conflicting alignment but without + other declarations inbetween. */ +inline int ATTR ((aligned (32))) +finline_align (int); /* { dg-message "note: previous declaration here" } */ + +inline int ATTR ((aligned (4))) +finline_align (int); /* { dg-warning "ignoring attribute .aligned \\(4\\). because it conflicts with attribute .aligned \\(32\\)." "" } */ + +inline int ATTR ((noreturn)) +finline_align (int i) { (void) __builtin_abort (); } + + +/* Expect no note that would refer to the same declaration. */ +inline int ATTR ((aligned (32), aligned (4))) +finline_double_align (int); /* { dg-warning "ignoring attribute .aligned \\(4\\). because it conflicts with attribute .aligned \\(32\\)." } */ + +inline int ATTR ((noreturn)) +finline_double_align (int i) { (void) __builtin_abort (); } + + /* Exercise variable attributes. */ extern int ATTR ((common)) diff --git a/gcc/testsuite/gcc.dg/Wattributes-6.c b/gcc/testsuite/gcc.dg/Wattributes-6.c index 4ba59bf2806..c6b2225943d 100644 --- a/gcc/testsuite/gcc.dg/Wattributes-6.c +++ b/gcc/testsuite/gcc.dg/Wattributes-6.c @@ -401,8 +401,7 @@ inline int ATTR ((warn_unused_result)) finline_hot_noret_align (int); /* {
Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.
As you say, the logic is convoluted. Let's simplify it rather than make it more convoluted. One possibility would be to change || to | to avoid the shortcut, and then bool note = lastalign > curalign; if (note) curalign = lastalign; I went with your suggestion in the attached v2. Regtested and bootstrapped on s390x, x86 and ppc64le. Regards Robin diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c index ccf9e4ccf0b..f9d1c17f8ef 100644 --- a/gcc/c-family/c-attribs.c +++ b/gcc/c-family/c-attribs.c @@ -2314,14 +2314,14 @@ common_handle_aligned_attribute (tree *node, tree name, tree args, int flags, *no_add_attrs = true; } else if (TREE_CODE (decl) == FUNCTION_DECL - && ((curalign = DECL_ALIGN (decl)) > bitalign - || ((lastalign = DECL_ALIGN (last_decl)) > bitalign))) + && (((curalign = DECL_ALIGN (decl)) > bitalign) + | ((lastalign = DECL_ALIGN (last_decl)) > bitalign))) { /* Either a prior attribute on the same declaration or one on a prior declaration of the same function specifies stricter alignment than this attribute. */ - bool note = lastalign != 0; - if (lastalign) + bool note = lastalign > curalign; + if (note) curalign = lastalign; curalign /= BITS_PER_UNIT; @@ -2366,25 +2366,6 @@ common_handle_aligned_attribute (tree *node, tree name, tree args, int flags, This formally comes from the c++11 specification but we are doing it for the GNU attribute syntax as well. */ *no_add_attrs = true; - else if (!warn_if_not_aligned_p - && TREE_CODE (decl) == FUNCTION_DECL - && DECL_ALIGN (decl) > bitalign) -{ - /* Don't warn for function alignment here if warn_if_not_aligned_p - is true. It will be warned about later. */ - if (DECL_USER_ALIGN (decl)) - { - /* Only reject attempts to relax/override an alignment - explicitly specified previously and accept declarations - that appear to relax the implicit function alignment for - the target. Both increasing and increasing the alignment - set by -falign-functions setting is permitted. */ - error ("alignment for %q+D was previously specified as %d " - "and may not be decreased", decl, - DECL_ALIGN (decl) / BITS_PER_UNIT); - *no_add_attrs = true; - } -} else if (warn_if_not_aligned_p && TREE_CODE (decl) == FIELD_DECL && !DECL_C_BIT_FIELD (decl)) diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c index 53b2b5b637d..40488585052 100644 --- a/gcc/c/c-decl.c +++ b/gcc/c/c-decl.c @@ -2620,6 +2620,9 @@ merge_decls (tree newdecl, tree olddecl, tree newtype, tree oldtype) SET_DECL_ALIGN (newdecl, DECL_ALIGN (olddecl)); DECL_USER_ALIGN (newdecl) |= DECL_USER_ALIGN (olddecl); } + else if (DECL_ALIGN (olddecl) == DECL_ALIGN (newdecl) + && DECL_USER_ALIGN (olddecl) != DECL_USER_ALIGN (newdecl)) + DECL_USER_ALIGN (newdecl) = 1; if (DECL_WARN_IF_NOT_ALIGN (olddecl) > DECL_WARN_IF_NOT_ALIGN (newdecl)) SET_DECL_WARN_IF_NOT_ALIGN (newdecl, diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c index e7268d5ad18..f774c75228d 100644 --- a/gcc/cp/decl.c +++ b/gcc/cp/decl.c @@ -2794,6 +2794,10 @@ duplicate_decls (tree newdecl, tree olddecl, bool hiding, bool was_hidden) SET_DECL_ALIGN (newdecl, DECL_ALIGN (olddecl)); DECL_USER_ALIGN (newdecl) |= DECL_USER_ALIGN (olddecl); } + else if (DECL_ALIGN (olddecl) == DECL_ALIGN (newdecl) + && DECL_USER_ALIGN (olddecl) != DECL_USER_ALIGN (newdecl)) +DECL_USER_ALIGN (newdecl) = 1; + DECL_USER_ALIGN (olddecl) = DECL_USER_ALIGN (newdecl); if (DECL_WARN_IF_NOT_ALIGN (olddecl) > DECL_WARN_IF_NOT_ALIGN (newdecl))
Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.
On 5/25/21 11:15 AM, Martin Sebor wrote: On 5/25/21 4:38 AM, Robin Dapp wrote: Hi Martin and Jason, The removal of the dead code looks good to me. The change to "re-init lastalign" doesn't seem right. When it's zero it means the conflict is between two attributes on the same declaration, in which case the note shouldn't be printed (it would just point to the same location as the warning). Agreed. Did I get it correctly that you refer to printing a note in e.g. the following case? inline int __attribute__ ((aligned (16), aligned (4))) finline_align (int); Yes, that's what I was referring to. I indeed missed this but it could be fixed by checking (on top of the patch) diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c index 98c98944405..7349da73f14 100644 --- a/gcc/c-family/c-attribs.c +++ b/gcc/c-family/c-attribs.c @@ -2324,7 +2324,7 @@ common_handle_aligned_attribute (tree *node, tree name, tree args, int flags, /* Either a prior attribute on the same declaration or one on a prior declaration of the same function specifies stricter alignment than this attribute. */ - bool note = lastalign != 0; + bool note = last_decl != decl && lastalign != 0; As there wasn't any FAIL, I would add another test which checks for this. That would be great, thank you! I find the whole logic here a bit convoluted but when there is no real last_decl, then last_decl = decl. A note would not be printed before the patch because we erroneously warned about the "conflict" of the function's default alignment (8) vs the requested alignment (4). Ah, the problem is that we only give this warning because of DECL_USER_ALIGN on last_decl, but then don't use the alignment of last_decl. As you say, the logic is convoluted. Let's simplify it rather than make it more convoluted. One possibility would be to change || to | to avoid the shortcut, and then bool note = lastalign > curalign; if (note) curalign = lastalign; Jason
Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.
On 5/25/21 4:38 AM, Robin Dapp wrote: Hi Martin and Jason, The removal of the dead code looks good to me. The change to "re-init lastalign" doesn't seem right. When it's zero it means the conflict is between two attributes on the same declaration, in which case the note shouldn't be printed (it would just point to the same location as the warning). Agreed. Did I get it correctly that you refer to printing a note in e.g. the following case? inline int __attribute__ ((aligned (16), aligned (4))) finline_align (int); Yes, that's what I was referring to. I indeed missed this but it could be fixed by checking (on top of the patch) diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c index 98c98944405..7349da73f14 100644 --- a/gcc/c-family/c-attribs.c +++ b/gcc/c-family/c-attribs.c @@ -2324,7 +2324,7 @@ common_handle_aligned_attribute (tree *node, tree name, tree args, int flags, /* Either a prior attribute on the same declaration or one on a prior declaration of the same function specifies stricter alignment than this attribute. */ - bool note = lastalign != 0; + bool note = last_decl != decl && lastalign != 0; As there wasn't any FAIL, I would add another test which checks for this. That would be great, thank you! Martin I find the whole logic here a bit convoluted but when there is no real last_decl, then last_decl = decl. A note would not be printed before the patch because we erroneously warned about the "conflict" of the function's default alignment (8) vs the requested alignment (4). Regards Robin
Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.
Hi Martin and Jason, The removal of the dead code looks good to me. The change to "re-init lastalign" doesn't seem right. When it's zero it means the conflict is between two attributes on the same declaration, in which case the note shouldn't be printed (it would just point to the same location as the warning). Agreed. Did I get it correctly that you refer to printing a note in e.g. the following case? inline int __attribute__ ((aligned (16), aligned (4))) finline_align (int); I indeed missed this but it could be fixed by checking (on top of the patch) diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c index 98c98944405..7349da73f14 100644 --- a/gcc/c-family/c-attribs.c +++ b/gcc/c-family/c-attribs.c @@ -2324,7 +2324,7 @@ common_handle_aligned_attribute (tree *node, tree name, tree args, int flags, /* Either a prior attribute on the same declaration or one on a prior declaration of the same function specifies stricter alignment than this attribute. */ - bool note = lastalign != 0; + bool note = last_decl != decl && lastalign != 0; As there wasn't any FAIL, I would add another test which checks for this. I find the whole logic here a bit convoluted but when there is no real last_decl, then last_decl = decl. A note would not be printed before the patch because we erroneously warned about the "conflict" of the function's default alignment (8) vs the requested alignment (4). Regards Robin
Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.
On 5/19/21 6:03 PM, Martin Sebor via Gcc-patches wrote: On 5/3/21 8:53 AM, Robin Dapp via Gcc-patches wrote: Hi, on s390 a warning test fails: inline int ATTR ((cold, aligned (8))) finline_hot_noret_align (int); inline int ATTR ((warn_unused_result)) finline_hot_noret_align (int); inline int ATTR ((aligned (4))) finline_hot_noret_align (int); /* { dg-warning "ignoring attribute .aligned \\(4\\). because it conflicts with attribute .aligned \\(8\\)." This test actually uncovered two problems. First, on s390 the default function alignment is 8 bytes. When the second decl above is merged with the first one, DECL_USER_ALIGN is only copied if DECL_ALIGN (old) > DECL_ALIGN (new). Subsequently, when merging the third decl, no warning is emitted since DECL_USER_ALIGN is unset. This patch also copies DECL_USER_ALIGN if DECL_ALIGN (old) == DECL_ALIGN (new) && DECL_USER_ALIGN (olddecl) != DECL_USER_ALIGN (newdecl)). Then, while going through the related files I also noticed that we emit a wrong warning for: inline int ATTR ((aligned (32))) finline_align (int); inline int ATTR ((aligned (4))) finline_align (int); /* { dg-warning "ignoring attribute .aligned \\(4\\). because it conflicts with attribute .aligned \\(32\\)." "" } */ What we emit is warning: ignoring attribute ‘aligned (4)’ because it conflicts with attribute ‘aligned (8)’ [-Wattributes]. This is due to the short circuit evaluation in c-attribs.c: && ((curalign = DECL_ALIGN (decl)) > bitalign || ((lastalign = DECL_ALIGN (last_decl)) > bitalign))) where lastalign is only initialized when curalign > bitalign. On s390 this is not the case and lastalign is used zero-initialized in the following code. On top, the following condition else if (!warn_if_not_aligned_p && TREE_CODE (decl) == FUNCTION_DECL && DECL_ALIGN (decl) > bitalign) seems to be fully covered by else if (TREE_CODE (decl) == FUNCTION_DECL && ((curalign = DECL_ALIGN (decl)) > bitalign || ((lastalign = DECL_ALIGN (last_decl)) > bitalign))) and so is essentially dead. I therefore removed it as there does not seem to be a test anywhere for the error message ( "alignment for %q+D was previously specified as %d and may not be decreased") either. The removal of the dead code looks good to me. The change to "re-init lastalign" doesn't seem right. When it's zero it means the conflict is between two attributes on the same declaration, in which case the note shouldn't be printed (it would just point to the same location as the warning). Agreed. diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c index c1f652d1dc9..d132b6fd3b6 100644 --- a/gcc/c-family/c-attribs.c +++ b/gcc/c-family/c-attribs.c @@ -2317,6 +2317,10 @@ common_handle_aligned_attribute (tree *node, tree name, tree args, int flags, && ((curalign = DECL_ALIGN (decl)) > bitalign || ((lastalign = DECL_ALIGN (last_decl)) > bitalign))) { + /* Re-init lastalign in case we short-circuit the condition, + i.e. curalign > bitalign. */ + lastalign = DECL_ALIGN (last_decl); + /* Either a prior attribute on the same declaration or one on a prior declaration of the same function specifies stricter alignment than this attribute. */ For the C/C++ FE changes: diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c index 3ea4708c507..2ea5051e9cd 100644 --- a/gcc/c/c-decl.c +++ b/gcc/c/c-decl.c @@ -2620,6 +2620,9 @@ merge_decls (tree newdecl, tree olddecl, tree newtype, tree oldtype) SET_DECL_ALIGN (newdecl, DECL_ALIGN (olddecl)); DECL_USER_ALIGN (newdecl) |= DECL_USER_ALIGN (olddecl); } + else if (DECL_ALIGN (olddecl) == DECL_ALIGN (newdecl) + && DECL_USER_ALIGN (olddecl) != DECL_USER_ALIGN (newdecl)) + DECL_USER_ALIGN (newdecl) |= DECL_USER_ALIGN (olddecl); This should be the same as DECL_USER_ALIGN (newdecl) = 1; so I would suggest to use that for clarity. I'm not sure that's clearer, though it certainly is equivalent. I'm happy either way. The braces around this statement in the parallel C++ change are unnecessary. I'm a little nervous about the front-end changes given that we were specifically not setting *_USER_ALIGN in this case before, but I'm not aware of any rationale for that, so I'm content to make the change and see if anything breaks. Jason
Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.
On 5/3/21 8:53 AM, Robin Dapp via Gcc-patches wrote: Hi, on s390 a warning test fails: inline int ATTR ((cold, aligned (8))) finline_hot_noret_align (int); inline int ATTR ((warn_unused_result)) finline_hot_noret_align (int); inline int ATTR ((aligned (4))) finline_hot_noret_align (int); /* { dg-warning "ignoring attribute .aligned \\(4\\). because it conflicts with attribute .aligned \\(8\\)." This test actually uncovered two problems. First, on s390 the default function alignment is 8 bytes. When the second decl above is merged with the first one, DECL_USER_ALIGN is only copied if DECL_ALIGN (old) > DECL_ALIGN (new). Subsequently, when merging the third decl, no warning is emitted since DECL_USER_ALIGN is unset. This patch also copies DECL_USER_ALIGN if DECL_ALIGN (old) == DECL_ALIGN (new) && DECL_USER_ALIGN (olddecl) != DECL_USER_ALIGN (newdecl)). Then, while going through the related files I also noticed that we emit a wrong warning for: inline int ATTR ((aligned (32))) finline_align (int); inline int ATTR ((aligned (4))) finline_align (int); /* { dg-warning "ignoring attribute .aligned \\(4\\). because it conflicts with attribute .aligned \\(32\\)." "" } */ What we emit is warning: ignoring attribute ‘aligned (4)’ because it conflicts with attribute ‘aligned (8)’ [-Wattributes]. This is due to the short circuit evaluation in c-attribs.c: && ((curalign = DECL_ALIGN (decl)) > bitalign || ((lastalign = DECL_ALIGN (last_decl)) > bitalign))) where lastalign is only initialized when curalign > bitalign. On s390 this is not the case and lastalign is used zero-initialized in the following code. On top, the following condition else if (!warn_if_not_aligned_p && TREE_CODE (decl) == FUNCTION_DECL && DECL_ALIGN (decl) > bitalign) seems to be fully covered by else if (TREE_CODE (decl) == FUNCTION_DECL && ((curalign = DECL_ALIGN (decl)) > bitalign || ((lastalign = DECL_ALIGN (last_decl)) > bitalign))) and so is essentially dead. I therefore removed it as there does not seem to be a test anywhere for the error message ( "alignment for %q+D was previously specified as %d and may not be decreased") either. The removal of the dead code looks good to me. The change to "re-init lastalign" doesn't seem right. When it's zero it means the conflict is between two attributes on the same declaration, in which case the note shouldn't be printed (it would just point to the same location as the warning). diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c index c1f652d1dc9..d132b6fd3b6 100644 --- a/gcc/c-family/c-attribs.c +++ b/gcc/c-family/c-attribs.c @@ -2317,6 +2317,10 @@ common_handle_aligned_attribute (tree *node, tree name, tree args, int flags, && ((curalign = DECL_ALIGN (decl)) > bitalign || ((lastalign = DECL_ALIGN (last_decl)) > bitalign))) { + /* Re-init lastalign in case we short-circuit the condition, +i.e. curalign > bitalign. */ + lastalign = DECL_ALIGN (last_decl); + /* Either a prior attribute on the same declaration or one on a prior declaration of the same function specifies stricter alignment than this attribute. */ For the C/C++ FE changes: diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c index 3ea4708c507..2ea5051e9cd 100644 --- a/gcc/c/c-decl.c +++ b/gcc/c/c-decl.c @@ -2620,6 +2620,9 @@ merge_decls (tree newdecl, tree olddecl, tree newtype, tree oldtype) SET_DECL_ALIGN (newdecl, DECL_ALIGN (olddecl)); DECL_USER_ALIGN (newdecl) |= DECL_USER_ALIGN (olddecl); } + else if (DECL_ALIGN (olddecl) == DECL_ALIGN (newdecl) + && DECL_USER_ALIGN (olddecl) != DECL_USER_ALIGN (newdecl)) + DECL_USER_ALIGN (newdecl) |= DECL_USER_ALIGN (olddecl); This should be the same as DECL_USER_ALIGN (newdecl) = 1; so I would suggest to use that for clarity. Other than that, a maintainer needs to review and approve the work. Martin
Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.
on s390 a warning test fails: inline int ATTR ((cold, aligned (8))) finline_hot_noret_align (int); inline int ATTR ((warn_unused_result)) finline_hot_noret_align (int); inline int ATTR ((aligned (4))) finline_hot_noret_align (int); /* { dg-warning "ignoring attribute .aligned \\(4\\). because it conflicts with attribute .aligned \\(8\\)." This test actually uncovered two problems. First, on s390 the default function alignment is 8 bytes. When the second decl above is merged with the first one, DECL_USER_ALIGN is only copied if DECL_ALIGN (old) > DECL_ALIGN (new). Subsequently, when merging the third decl, no warning is emitted since DECL_USER_ALIGN is unset. [..] Ping.
Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.
Ping.
[PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.
Hi, on s390 a warning test fails: inline int ATTR ((cold, aligned (8))) finline_hot_noret_align (int); inline int ATTR ((warn_unused_result)) finline_hot_noret_align (int); inline int ATTR ((aligned (4))) finline_hot_noret_align (int); /* { dg-warning "ignoring attribute .aligned \\(4\\). because it conflicts with attribute .aligned \\(8\\)." This test actually uncovered two problems. First, on s390 the default function alignment is 8 bytes. When the second decl above is merged with the first one, DECL_USER_ALIGN is only copied if DECL_ALIGN (old) > DECL_ALIGN (new). Subsequently, when merging the third decl, no warning is emitted since DECL_USER_ALIGN is unset. This patch also copies DECL_USER_ALIGN if DECL_ALIGN (old) == DECL_ALIGN (new) && DECL_USER_ALIGN (olddecl) != DECL_USER_ALIGN (newdecl)). Then, while going through the related files I also noticed that we emit a wrong warning for: inline int ATTR ((aligned (32))) finline_align (int); inline int ATTR ((aligned (4))) finline_align (int); /* { dg-warning "ignoring attribute .aligned \\(4\\). because it conflicts with attribute .aligned \\(32\\)." "" } */ What we emit is warning: ignoring attribute ‘aligned (4)’ because it conflicts with attribute ‘aligned (8)’ [-Wattributes]. This is due to the short circuit evaluation in c-attribs.c: && ((curalign = DECL_ALIGN (decl)) > bitalign || ((lastalign = DECL_ALIGN (last_decl)) > bitalign))) where lastalign is only initialized when curalign > bitalign. On s390 this is not the case and lastalign is used zero-initialized in the following code. On top, the following condition else if (!warn_if_not_aligned_p && TREE_CODE (decl) == FUNCTION_DECL && DECL_ALIGN (decl) > bitalign) seems to be fully covered by else if (TREE_CODE (decl) == FUNCTION_DECL && ((curalign = DECL_ALIGN (decl)) > bitalign || ((lastalign = DECL_ALIGN (last_decl)) > bitalign))) and so is essentially dead. I therefore removed it as there does not seem to be a test anywhere for the error message ( "alignment for %q+D was previously specified as %d and may not be decreased") either. Bootstrapped and regtested on s390, ppc64 and amd64. Is it OK? Regards Robin -- gcc/c-family/ChangeLog: * c-attribs.c (common_handle_aligned_attribute): Re-init lastalign and remove case. gcc/c/ChangeLog: * c-decl.c (merge_decls): Copy DECL_USER_ALIGN even for similar DECL_ALIGN. gcc/cp/ChangeLog: * decl.c (duplicate_decls): Dito. >From f3a9f8b9bd1e28c23f2efcdb061959852a91deb6 Mon Sep 17 00:00:00 2001 From: Robin Dapp Date: Tue, 20 Apr 2021 10:51:18 +0200 Subject: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar. When re-declaring a function with differing attributes DECL_USER_ALIGN is usually not merged/copied when DECL_ALIGN is similar. On s390 this will cause a warning message not to be shown. Similarly, a note is not shown when we short-circuit an alignment initialization in common_handle_aligned_attribute (). Fix this by copying DECL_USER_ALIGN even if DECL_ALIGN is similar as well as reinitializing the short-circuited initialization. --- gcc/c-family/c-attribs.c | 23 --- gcc/c/c-decl.c | 3 +++ gcc/cp/decl.c| 5 + 3 files changed, 12 insertions(+), 19 deletions(-) diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c index c1f652d1dc9..d132b6fd3b6 100644 --- a/gcc/c-family/c-attribs.c +++ b/gcc/c-family/c-attribs.c @@ -2317,6 +2317,10 @@ common_handle_aligned_attribute (tree *node, tree name, tree args, int flags, && ((curalign = DECL_ALIGN (decl)) > bitalign || ((lastalign = DECL_ALIGN (last_decl)) > bitalign))) { + /* Re-init lastalign in case we short-circuit the condition, + i.e. curalign > bitalign. */ + lastalign = DECL_ALIGN (last_decl); + /* Either a prior attribute on the same declaration or one on a prior declaration of the same function specifies stricter alignment than this attribute. */ @@ -2366,25 +2370,6 @@ common_handle_aligned_attribute (tree *node, tree name, tree args, int flags, This formally comes from the c++11 specification but we are doing it for the GNU attribute syntax as well. */ *no_add_attrs = true; - else if (!warn_if_not_aligned_p - && TREE_CODE (decl) == FUNCTION_DECL - && DECL_ALIGN (decl) > bitalign) -{ - /* Don't warn for function alignment here if warn_if_not_aligned_p - is true. It will be warned about later. */ - if (DECL_USER_ALIGN (decl)) - { - /* Only reject attempts to relax/override an alignment - explicitly specified previously and accept declarations - that appear to relax the implicit function alignment for - the ta