Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.

2021-06-09 Thread Jason Merrill via Gcc-patches

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.

2021-06-09 Thread Robin Dapp via Gcc-patches

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.

2021-06-01 Thread Jason Merrill via Gcc-patches

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.

2021-06-01 Thread Robin Dapp via Gcc-patches
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.

2021-06-01 Thread Robin Dapp via Gcc-patches

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.

2021-05-25 Thread Jason Merrill via Gcc-patches

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.

2021-05-25 Thread Martin Sebor via Gcc-patches

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.

2021-05-25 Thread Robin Dapp via Gcc-patches

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.

2021-05-21 Thread Jason Merrill via Gcc-patches

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.

2021-05-19 Thread Martin Sebor via Gcc-patches

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.

2021-05-17 Thread Robin Dapp via Gcc-patches

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.

2021-05-10 Thread Robin Dapp via Gcc-patches

Ping.


[PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.

2021-05-03 Thread Robin Dapp via Gcc-patches

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