User account creation filtered due to spam.

Bug 77728 - [5 Regression] Miscompilation multiple vector iteration on ARM
Summary: [5 Regression] Miscompilation multiple vector iteration on ARM
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 6.2.1
: P2 normal
Target Milestone: 5.5
Assignee: Not yet assigned to anyone
URL:
Keywords: ABI, wrong-code
: 69841 80149 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-09-25 01:32 UTC by Yichao Yu
Modified: 2017-05-10 11:12 UTC (History)
14 users (show)

See Also:
Host:
Target: arm
Build:
Known to work: 4.9.4
Known to fail: 5.1.0, 6.2.0
Last reconfirmed: 2016-09-26 00:00:00


Attachments
AArch64 prototype. (1.71 KB, patch)
2017-04-20 21:52 UTC, Ramana Radhakrishnan
Details | Diff
AArch64 patch .. (1.60 KB, patch)
2017-04-24 13:04 UTC, Ramana Radhakrishnan
Details | Diff
AArch32 wip patch. (1.66 KB, text/plain)
2017-04-24 13:11 UTC, Ramana Radhakrishnan
Details
AArch32 wip patch. (1.69 KB, text/plain)
2017-04-24 16:03 UTC, Ramana Radhakrishnan
Details
gcc7-pr77728.patch (1.59 KB, patch)
2017-04-24 17:59 UTC, Jakub Jelinek
Details | Diff
gcc7-pr77728.patch (2.52 KB, patch)
2017-04-25 08:02 UTC, Jakub Jelinek
Details | Diff
gcc7-pr77728.patch (2.63 KB, patch)
2017-04-25 08:20 UTC, Jakub Jelinek
Details | Diff
gcc7-pr77728.patch (2.78 KB, patch)
2017-04-25 09:13 UTC, Jakub Jelinek
Details | Diff
gcc7-pr77728-aarch64.patch (2.74 KB, patch)
2017-04-25 09:25 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yichao Yu 2016-09-25 01:32:58 UTC
Code to reproduce is at https://gist.github.com/yuyichao/a66edb9d05d18755fb7587b12e021a8a. The two cpp files are

```c++
#include <stdint.h>
#include <vector>

typedef std::vector<std::pair<uint64_t, uint64_t>> DWARFAddressRangesVector;

void dumpRanges(const DWARFAddressRangesVector& Ranges) {
    for (const auto &Range: Ranges) {
        (void)Range;
    }
}

void collectChildrenAddressRanges(DWARFAddressRangesVector& Ranges)
{
    const DWARFAddressRangesVector &DIERanges = DWARFAddressRangesVector();
    Ranges.insert(Ranges.end(), DIERanges.begin(), DIERanges.end());
}
```

```c++
#include <stdint.h>
#include <vector>

typedef std::vector<std::pair<uint64_t, uint64_t>> DWARFAddressRangesVector;

void collectAddressRanges(DWARFAddressRangesVector &CURanges,
                          const DWARFAddressRangesVector &CUDIERanges)
{
    CURanges.insert(CURanges.end(), CUDIERanges.begin(), CUDIERanges.end());
}

int main()
{
    std::vector<std::pair<uint64_t, uint64_t>> CURanges;
    std::vector<std::pair<uint64_t, uint64_t>> CUDIERanges{{1, 2}};
    collectAddressRanges(CURanges, CUDIERanges);
    return 0;
}
```

Both compiled with `g++ -O2` and linked together. When running the compiled program, it raises an exception in the `insert`

```
terminate called after throwing an instance of 'std::length_error'
  what():  vector::_M_range_insert
```

which shouldn't happen. The issue seems to be related to merging duplicated code since it is important to put the code into two files and the present of the second .o file is important even though none of the code in it is used. The iterations also have to be all on the const reference of vector. Removing one of the const also makes the issue go away.

The g++ is version 6.2.1 from the ArchLinuxARM armv7h repository. This might be a regression in gcc 5 since other devs using gcc <=4.9 doesn't seem to have this issue and I was able to reproduce this on archlinux on 4-5 different systems with gcc >=5.

This causes https://github.com/JuliaLang/julia/issues/14550
Comment 1 ktkachov 2016-09-26 16:50:18 UTC
Confirmed the exception on GCC 5 and later
Comment 2 Yichao Yu 2016-09-26 16:53:25 UTC
I should add that turning on lto works around the issue both in the simple code attached and for the original issue I was having in julia (i.e. compiling llvm with LTO makes the issue go away).
Comment 3 ktkachov 2016-09-27 12:42:45 UTC
Started with r225465.
Something to do with alignment.
I wonder if it's related to PR69841 ?
Comment 4 Yichao Yu 2016-10-20 13:12:53 UTC
Ping. Anything I can help with debugging this?
Comment 5 Yichao Yu 2017-01-13 18:03:06 UTC
Ping again? Anything new or I can help with here?
Comment 6 Yichao Yu 2017-03-15 15:22:21 UTC
Anything new here?
Comment 7 Jakub Jelinek 2017-04-12 08:26:55 UTC
It fails only if the (unused) *.o file is linked before the used one.

I'm seeing following difference in 
 _ZNSt6vectorISt4pairIyyESaIS1_EE15_M_range_insertIN9__gnu_cxx17__normal_iteratorIPKS1_S3_EEEEvNS6_IPS1_S3_EET_SC_St20forward_iterator_tag:
        .fnstart
-.LFB833:
-       @ args = 8, pretend = 0, frame = 8
+.LFB856:
+       @ args = 4, pretend = 0, frame = 8
        @ frame_needed = 0, uses_anonymous_args = 0
...

in a comdat section.
Comment 8 Jakub Jelinek 2017-04-12 09:33:39 UTC
Ugh, this seems to be a major ABI issue.
The following patch ought to fix that:
--- gcc/config/arm.c.jj	2017-04-11 19:06:53.000000000 +0200
+++ gcc/config/arm.c	2017-04-12 10:50:29.030494109 +0200
@@ -6472,7 +6472,7 @@ arm_needs_doubleword_align (machine_mode
 
   /* Record/aggregate types: Use greatest member alignment of any member.  */ 
   for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
-    if (DECL_ALIGN (field) > PARM_BOUNDARY)
+    if (TREE_CODE (field) == FIELD_DECL && DECL_ALIGN (field) > PARM_BOUNDARY)
       return true;
 
   return false;

at least the assembly (ignoring label numbers) for the comdat sections is then identical, for the second TU it is actually identical also with the assembly generated with unpatched compiler.
The problem is that TYPE_FIELDS in C++ don't contain just FIELD_DECLs, but lots of other stuff, e.g. TYPE_DECLs, CONST_DECLs, FUNCTION_DECLs.
And apparently in between the
__gnu_cxx::__normal_iterator<std::pair<unsigned long long, unsigned long long>*, std::vector<std::pair<unsigned long long, unsigned long long>, std::allocator<std::pair<unsigned long long, unsigned long long> > > >
type ends up with the value_type typedef in one of the TUs with DECL_ALIGN of 8, in another one of 64 and that causes the difference.  Don't know exactly why that happens, there is C++ template instantiation and type canonicalization in play.

Now, AAPCS latest version seems to say:
"A Composite Type is a collection of one or more Fundamental Data Types that are handled as a single entity at the procedure call level. A Composite Type can be any of:
- An aggregate, where the members are laid out sequentially in memory"
...
"- The natural alignment of a composite type is the maximum of each of the member alignments of the 'top-level' members of the composite type i.e. before any alignment adjustment of the entire composite is applied"

The important question here is whether "member" in the above text for C++ classes just stands for non-static data members (i.e. FIELD_DECLs) or if it stands for any members of the class.  I hope it talks just about the FIELD_DECLs, talking say about static data members or member functions "laid out sequentially in memory" doesn't make any sense, in that case the above would be a bugfix to make GCC match the AAPCS wording.  If not, the question is what other compilers do, and whether it is supportable at all that way.

The patch changes ABI e.g. for:
struct A { int i; static int k __attribute__((aligned (8))); };

A
foo (int x, A y)
{
  return y;
}

The difference between unpatched and patched compiler is:
-	mov	r0, r2
+	mov	r0, r1

clang 4.0.0 (trunk 291659) generates the same code as patched gcc.
Comment 9 Jakub Jelinek 2017-04-12 09:34:26 UTC
(In reply to Jakub Jelinek from comment #8)
> The problem is that TYPE_FIELDS in C++ don't contain just FIELD_DECLs, but
> lots of other stuff, e.g. TYPE_DECLs, CONST_DECLs, FUNCTION_DECLs.

Forgot about VAR_DECLs in this list (i.e. static data members).
Comment 10 Jakub Jelinek 2017-04-12 09:41:32 UTC
aarch64_function_arg_alignment
has:
  for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
    alignment = std::max (alignment, DECL_ALIGN (field));
and thus has similar problem (no testcases in this case though).
Comment 11 Ramana Radhakrishnan 2017-04-12 09:54:04 UTC
(In reply to Jakub Jelinek from comment #10)
> aarch64_function_arg_alignment
> has:
>   for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
>     alignment = std::max (alignment, DECL_ALIGN (field));
> and thus has similar problem (no testcases in this case though).

This is related to the fix for PR65956 that was in both the backends and thus any fix needs to go to both places. 

Richard - could you take a look at this please ? What Jakub says in comment #8 makes sense.
Comment 12 Jakub Jelinek 2017-04-12 11:08:41 UTC
I had a brief look at why there is the difference in DECL_ALIGN on TYPE_DECL
in this case, and the difference seems to come up from whether the type size/layout has been finalized before calling layout_decl on the corresponding TYPE_DECL or not.  The question is if we ever except for the arm/aarch64 bug ever care about DECL_ALIGN on TYPE_DECLs, if yes, perhaps finalize_type_size
should check if type or any variant's TYPE_NAME is a TYPE_DECL and in that case relayout_decl it?
Comment 13 rguenther@suse.de 2017-04-12 11:12:16 UTC
On Wed, 12 Apr 2017, jakub at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77728
> 
> Jakub Jelinek <jakub at gcc dot gnu.org> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                  CC|                            |jason at gcc dot gnu.org
> 
> --- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> I had a brief look at why there is the difference in DECL_ALIGN on TYPE_DECL
> in this case, and the difference seems to come up from whether the type
> size/layout has been finalized before calling layout_decl on the corresponding
> TYPE_DECL or not.  The question is if we ever except for the arm/aarch64 bug
> ever care about DECL_ALIGN on TYPE_DECLs, if yes, perhaps finalize_type_size
> should check if type or any variant's TYPE_NAME is a TYPE_DECL and in that case
> relayout_decl it?

IMHO TYPE_DECL shouldn't even _have_ DECL_ALIGN ...
Comment 14 Jakub Jelinek 2017-04-12 13:25:29 UTC
For the base class alignment vs. alignment of base class members, I think testcases could be e.g. like:
struct B { char a __attribute__((aligned (8))); };
struct A : public B {};

A
foo (int x, A y)
{
  return y;
}

or
struct __attribute__((aligned (8))) B { char a; };
struct A : public B {};

A
foo (int x, A y)
{
  return y;
}
and similar, though it seems g++ and clang++ agree on the passing at least in these two testcases.  Of course, one should also try multiple inheritance, etc.
Virtual inheritance probably doesn't matter, I think those types must be passed always by invisible reference.
Comment 15 Jason Merrill 2017-04-12 22:10:58 UTC
(In reply to rguenther@suse.de from comment #13)
> IMHO TYPE_DECL shouldn't even _have_ DECL_ALIGN ...

Agreed.
Comment 16 Ramana Radhakrishnan 2017-04-20 21:50:52 UTC
(In reply to Jakub Jelinek from comment #10)
> aarch64_function_arg_alignment
> has:
>   for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
>     alignment = std::max (alignment, DECL_ALIGN (field));
> and thus has similar problem (no testcases in this case though).

A prototype fix (needs tidying up for sure!) that warns for aarch64 is as attached. There are no regressions on a full bootstrap and test run. I would be grateful for any comments.

Note that I haven't managed to actually trigger an ABI warning because of this on aarch64. 

Note that this is dependent on clarification with respect to the AAPCS . 

I'm not yet happy with the amount of refactoring we have to do to get the warnings on ARM.


regards
Ramana
Comment 17 Ramana Radhakrishnan 2017-04-20 21:52:42 UTC
Created attachment 41237 [details]
AArch64 prototype.

Patch.
Comment 18 Jakub Jelinek 2017-04-21 07:10:27 UTC
If you want a testcase for aarch64, e.g.
template <int N>
struct alignas (16) A { char p[16]; };
A<0> v;
template <int N>
struct B
{
  typedef A<N> T;
  int i, j, k, l;
};
int
foo (int a, B<0> b)
{
  return a + b.i;
}
int
bar (int a, B<1> b)
{
  return a + b.i;
}
struct C
{
  static int h alignas (16);
  int i, j, k, l;
};
int
baz (int a, C b)
{
  return a + b.i;
}

will do the job, there is a warning on foo's b argument, because A<0> has been instantiated first and when instantiating B<0>, we already see A<0>'s alignment and use it when creating the TYPE_DECL.  Compared to that, for B<1> we don't have it instantiated yet, so when TYPE_DECL is created, it gets minimal alignment.  You might want to create e.g. a compat testcase from it too, have caller of foo with the A<0> v; in it and callee without and vice versa, check if it is able to pass the argument around properly.  Similarly with baz (in that case it actually was a stable (but wrong) ABI before, the baz caller and callee would agree, but they now don't in between unpatched and patched GCC.
For ARM you can easily adjust the testcase by s/16/8/g, and maybe removing k and l fields too.

Comments on the patch:

#include "print-tree.h"
you don't really need this, that is from some debugging, right?

  unsigned int alignment ; // alignment for FIELD_DECLS in function arguments.
extraneous space before ;, not sure if it is a good idea to mix // and /* comments and on the next line it is way too long, so maybe better use /* */
comments above each field.

 {
+
+  unsigned int alignment = 0;
+  unsigned int warning_alignment = 0;
extraneous vertical space.  Also, what is the point to have a struct containing
those 2 fields and auto vars mirroring that?  Just use the struct all the time?

alignment = std::max (alignment, DECL_ALIGN (field));
I think we usually use MAX rather than std::max.

+      if (nregs == 2 && (a.warning_alignment == 16 * BITS_PER_UNIT) && ncrn % 2)
+	  if (a.warning_alignment < a.alignment
+	      && warn_psabi)
+	    warning (0, "Parameter passing changed for argument");
The if condition fits on one line.  Do you need the ()s around a.warning_alignment == 16 * BITS_PER_UNIT?  Can't it be written as
      if (warn_psabi
          && nregs == 2
          && a.warning_alignment == 16 * BITS_PER_UNIT
          && a.alignment != 16 * BITS_PER_UNIT
          && ncrn % 2)
?  That is what is actually tested originally, whether alignment == 16 * BITS_PER_UNIT.  Also, warnings should not start with capital letter.
And it would be nice to tell user which argument it is (does cumulative_args_t track the argument number or could it?).

 on_stack:
   pcum->aapcs_stack_words = size / UNITS_PER_WORD;
-  if (aarch64_function_arg_alignment (mode, type) == 16 * BITS_PER_UNIT)
+  struct aarch64_fn_arg_alignment ab = aarch64_function_arg_alignment (mode, type);
+  if (ab.alignment == 16 * BITS_PER_UNIT)
     pcum->aapcs_stack_size = ROUND_UP (pcum->aapcs_stack_size,
 				       16 / UNITS_PER_WORD);
No warning here, if ab.alignment is not 16 * BITS_PER_UNIT, but ab.warning_alignment is and warn_psabi is on?  I must say I don't really know exactly what pcum->aapcs_stack_size influence, but doesn't it influence the ABI when the function is varargs?  I mean, with the above testcase, something like:
void
foo (int a, int b, int c, int d, int e, int f, int g, int h, int i, int j, int k, int l, int m, B<0> n, ...)
with/without the A<0> v; early?

   if (alignment < PARM_BOUNDARY)
     alignment = PARM_BOUNDARY;
   if (alignment > STACK_BOUNDARY)
     alignment = STACK_BOUNDARY;
+
+  if (a.warning_alignment < PARM_BOUNDARY)
+    a.warning_alignment = PARM_BOUNDARY;
+
+  if (a.warning_alignment > STACK_BOUNDARY)
+    a.warning_alignment = STACK_BOUNDARY;
+
Isn't the above candidate for MAX/MIN to put it into fewer lines?

+  if (a.warning_alignment > alignment
+      && warn_psabi)
+    warning (0, "Parameter passing changed for argument in function_arg_boundary");
+
+  

Again, condition can fit onto a single line.  Due to MAX/MIN, I guess the only possible values of those 2 are 64 and 128, so the comparison is fine.  But again, warning should not start with a capital letter and mentioning "in function_arg_boundary" is not helpful to the users, much better would be to tell which argument it is or something similar.  And, perhaps remove the alignment variable and always use a.alignment?  There is also extra vertical space before return.

+  align =  a.alignment / BITS_PER_UNIT;
Extraneous whitespace after =.
Comment 19 Jakub Jelinek 2017-04-21 07:47:09 UTC
Indeed, with the above plus:
#include <stdarg.h>

int
fn1 (int a, int b, int c, int d, int e, int f, int g, int h, int i, int j, int k, int l, int m, B<0> n, ...)
{
  va_list ap;
  va_start (ap, n);
  int x = va_arg (ap, int);
  va_end (ap);
  return x;
}

int
fn2 (int a, int b, int c, int d, int e, int f, int g, int h, int i, int j, int k, int l, int m,
B<1> n, ...)
{
  va_list ap;
  va_start (ap, n);
  int x = va_arg (ap, int);
  va_end (ap);
  return x;
}

there is ABI change for fn1 but not fn2.  But we already get a warning with your patch on that:
warning: Parameter passing changed for argument in function_arg_boundary
so perhaps no need to emit it for the second time.
Comment 20 Jakub Jelinek 2017-04-21 08:59:21 UTC
BTW, the wording e.g. i386 backend has is:
              inform (input_location,
                      "the ABI of passing structure with complex float"
                      " member has changed in GCC 4.4");
In this arm/aarch64 case, the wording can't be as specific, because it isn't all arguments of this type, but only some arguments of certain type (at some positions; for some types like those that have more aligned static data members it is all such positioned arguments, for others, e.g. the templates where the alignment used to depend on instantiation vs. non-instantiation something earlier, only some), but you should consider using inform rather than warning for consistency, and you should mention the GCC version where it has changed and maybe also print the type in the diagnostic, so that user at least knows what type it is that is problematic (but as it is not something as simple as in the i386 case, there should not be a single inform per TU, but about each case this is encountered).

Richard said we should defer RC1 till this is done, do you think you can have a patch for both architectures written today, tested over the weekend, so that we could do RC1 on Monday or Tuesday at latest?
Is there an agreement in ARM that what the patch does (for aarch64, and similar one for arm) is what the AAPCS says?
Comment 21 Jakub Jelinek 2017-04-21 09:41:14 UTC
inform, beyond being consistent with what i386 and rs6000 backends do here, has the advantage that it doesn't break the build even with -Werror, generally there is nothing wrong on the code we want to inform user about, so there is nothing to fix unless they want to achieve ABI compatibility with code compiled by older GCC versions.  For this category i386/rs6000 backends use inform.  There are also warning uses with OPT_Wpsabi, but those are there for when user is doing something considered wrong, e.g. passing vector argument by value in a function without corresponding ISA supporting those vectors enabled.
Comment 22 Ramana Radhakrishnan 2017-04-21 14:21:49 UTC
(In reply to Jakub Jelinek from comment #20)
> BTW, the wording e.g. i386 backend has is:
>               inform (input_location,
>                       "the ABI of passing structure with complex float"
>                       " member has changed in GCC 4.4");
> In this arm/aarch64 case, the wording can't be as specific, because it isn't
> all arguments of this type, but only some arguments of certain type (at some
> positions; for some types like those that have more aligned static data
> members it is all such positioned arguments, for others, e.g. the templates
> where the alignment used to depend on instantiation vs. non-instantiation
> something earlier, only some), but you should consider using inform rather
> than warning for consistency, and you should mention the GCC version where
> it has changed and maybe also print the type in the diagnostic, so that user
> at least knows what type it is that is problematic (but as it is not
> something as simple as in the i386 case, there should not be a single inform
> per TU, but about each case this is encountered).
> 
> Richard said we should defer RC1 till this is done, do you think you can
> have a patch for both architectures written today, tested over the weekend,
> so that we could do RC1 on Monday or Tuesday at latest?

I'll see what I can do but it's going to be tough to finish that by today.

> Is there an agreement in ARM that what the patch does (for aarch64, and
> similar one for arm) is what the AAPCS says?

 I don't see agreement being reached until next week. Sorry about the delay but it's just bad timing unfortunately.
Comment 23 Ramana Radhakrishnan 2017-04-24 13:04:10 UTC
Created attachment 41254 [details]
AArch64 patch ..

Most review comments addressed but missed the min / max in aarch64_function_arg_boundary.

Tests cleanly.
Comment 24 Jakub Jelinek 2017-04-24 13:07:55 UTC
std::min/max was used in the backend previously too, so it is your decision and doesn't need to be changed to resolve this regression.
Do you have arm32 patch too, or would it be helpful if I write one similarly to the aarch64 one (can't test easily though)?
Any ETA when the ABI can be discussed inside of ARM?
Comment 25 Ramana Radhakrishnan 2017-04-24 13:11:03 UTC
Created attachment 41255 [details]
AArch32 wip patch.

- There are some line > 80 characters, cosmetic issues in this patch . 

- However this appeared to cause regressions in libstdc++ testruns over the weekend that Iv'e not been able to debug / fix up especially with precompiled headers with the configuration --with-arch=armv7-a --with-float=hard --with-fpu=neon.

- I've put this up for some comments and any reviews / additional testing. The warnings show up on various testcases from the PR.
Comment 26 Ramana Radhakrishnan 2017-04-24 16:03:30 UTC
Created attachment 41257 [details]
AArch32 wip patch.

I think there's one line > 80 chars here but this is what I'm testing currently.

The failures in libstdc++ arise only in a bootstrapped compiler which indicates a miscompiled compiler that isn't caught by bootstrap.
Comment 27 Ramana Radhakrishnan 2017-04-24 16:33:56 UTC
(In reply to Ramana Radhakrishnan from comment #26)
> Created attachment 41257 [details]
> AArch32 wip patch.
> 
> I think there's one line > 80 chars here but this is what I'm testing
> currently.
> 
> The failures in libstdc++ arise only in a bootstrapped compiler which
> indicates a miscompiled compiler that isn't caught by bootstrap.

I'm doing a full clean bootstrap and test run with this on aarch32, so lets see what we get.
Comment 28 Jakub Jelinek 2017-04-24 17:27:22 UTC
The
+         else
+             warn_align = MAX (warn_align, DECL_ALIGN (field));
last line here is misindented, should be below se.
Comment 29 Jakub Jelinek 2017-04-24 17:31:28 UTC
Also, the aarch64_function_arg_alignment and arm_needs_doubleword_align function comments need updating.
Comment 30 Jakub Jelinek 2017-04-24 17:59:31 UTC
Created attachment 41259 [details]
gcc7-pr77728.patch

This is how I'd have written the aarch32 patch.  It has smaller amount of changes and calls the sometimes expensive function only if really needed, not always.  Though, if there are libstdc++ failures, we also need some -Wno-psabi addition somewhere, perhaps:
--- libstdc++-v3/testsuite/lib/libstdc++.exp 2017-01-16 12:27:56.178985247 +0100
+++ libstdc++-v3/testsuite/lib/libstdc++.exp 2017-04-24 19:58:39.784972214 +0200
@@ -239,7 +239,7 @@ proc libstdc++_init { testfile } {
 
     # Default settings.
     set cxx [transform "g++"]
-    set cxxflags "-fmessage-length=0"
+    set cxxflags "-fmessage-length=0 -Wno-psabi"
     set cxxpchflags ""
     set cxxvtvflags ""
     set cxxldflags ""
Comment 31 Jakub Jelinek 2017-04-24 22:44:52 UTC
(In reply to Jakub Jelinek from comment #30)
> Created attachment 41259 [details]
> gcc7-pr77728.patch

The above attached version successfully bootstrapped with
--enable-languages=c,c++ --enable-checking=release --with-tune=cortex-a8 --with-arch=armv7-a --with-float=hard --with-fpu=vfpv3-d16 --with-abi=aapcs-linux
Regtest ongoing.
Comment 32 Jakub Jelinek 2017-04-25 08:02:57 UTC
Created attachment 41261 [details]
gcc7-pr77728.patch

Updated AArch32 patch, including testcase.  It bootstrapped/regtested fine, no changes were needed in libstdc++/testsuite/lib/ - note: is pruned out by default (gcc-dg.exp).
But for some reason no notes are reported in the caller of fn1 and fn3 where I'd expect them.
Comment 33 Jakub Jelinek 2017-04-25 08:20:37 UTC
Created attachment 41262 [details]
gcc7-pr77728.patch

Further updated AArch32 patch, which makes sure we warn even in the callers of the functions that changed ABI, but also makes sure we don't warn about them twice and only warn if we are actually emitting those calls.
Comment 34 Jakub Jelinek 2017-04-25 09:13:20 UTC
Created attachment 41263 [details]
gcc7-pr77728.patch

Further tweak, so that we don't emit the note twice on fn1 and fn3 in the testcase.  Seems during expand_function_start, we only want to warn in FUNCTION_ARG_BOUNDARY hook, because all the interesting cases are seen there, while not in FUNCTION_ARG hook.  But when in expand_call, we need FUNCTION_ARG.
Comment 35 Jakub Jelinek 2017-04-25 09:25:08 UTC
Created attachment 41264 [details]
gcc7-pr77728-aarch64.patch

Similarly adjusted AArch64 patch.
In the earlier AArch64 patch, warning_alignment didn't match the comment
(it was set to alignment if there were no other decls but FIELD_DECLs or when not a structure, so effectively warning_alignment was guaranteed to be >= alignment), and then there was: if (a.warning_alignment < a.alignment
which would therefore never be true.  But, we want to actually note if
warning_alignment is 16 bytes but alignment is smaller than that (because that is the case when we previously returned 16 and now have alignment that is not 16).
Comment 36 Jakub Jelinek 2017-04-25 13:53:05 UTC
Author: jakub
Date: Tue Apr 25 13:52:33 2017
New Revision: 247239

URL: https://gcc.gnu.org/viewcvs?rev=247239&root=gcc&view=rev
Log:
	PR target/77728
	* config/aarch64/aarch64.c (struct aarch64_fn_arg_alignment): New
	type.
	(aarch64_function_arg_alignment): Return aarch64_fn_arg_alignment
	struct.  Ignore DECL_ALIGN of decls other than FIELD_DECL for
	the alignment computation, but return their maximum in warn_alignment.
	(aarch64_layout_arg): Adjust aarch64_function_arg_alignment caller.
	Emit a -Wpsabi note if warn_alignment is 16 bytes, but alignment
	is smaller.
	(aarch64_function_arg_boundary): Likewise.  Simplify using MIN/MAX.
	(aarch64_gimplify_va_arg_expr): Adjust aarch64_function_arg_alignment
	caller.
testsuite/
	* g++.dg/abi/pr77728-2.C: New test.

Added:
    trunk/gcc/testsuite/g++.dg/abi/pr77728-2.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/aarch64/aarch64.c
    trunk/gcc/testsuite/ChangeLog
Comment 37 Jakub Jelinek 2017-04-25 13:56:42 UTC
Author: jakub
Date: Tue Apr 25 13:56:10 2017
New Revision: 247241

URL: https://gcc.gnu.org/viewcvs?rev=247241&root=gcc&view=rev
Log:
	PR target/77728
	* config/aarch64/aarch64.c (struct aarch64_fn_arg_alignment): New
	type.
	(aarch64_function_arg_alignment): Return aarch64_fn_arg_alignment
	struct.  Ignore DECL_ALIGN of decls other than FIELD_DECL for
	the alignment computation, but return their maximum in warn_alignment.
	(aarch64_layout_arg): Adjust aarch64_function_arg_alignment caller.
	Emit a -Wpsabi note if warn_alignment is 16 bytes, but alignment
	is smaller.
	(aarch64_function_arg_boundary): Likewise.  Simplify using MIN/MAX.
	(aarch64_gimplify_va_arg_expr): Adjust aarch64_function_arg_alignment
	caller.
testsuite/
	* g++.dg/abi/pr77728-2.C: New test.

Added:
    branches/gcc-7-branch/gcc/testsuite/g++.dg/abi/pr77728-2.C
Modified:
    branches/gcc-7-branch/gcc/ChangeLog
    branches/gcc-7-branch/gcc/config/aarch64/aarch64.c
    branches/gcc-7-branch/gcc/testsuite/ChangeLog
Comment 38 Christophe Lyon 2017-04-25 15:17:07 UTC
Is there a plan to backport this fix to the 5 and 6 branches?
Comment 39 Jakub Jelinek 2017-04-25 15:20:29 UTC
It is an ABI change, so I think it is highly undesirable to backport.  It is enough that people will have to rebuild many packages built by GCC 7 prereleases, if it is backported, they would have to rebuild also anything built by GCC 5 or 6.
Comment 40 Richard Earnshaw 2017-04-25 15:30:02 UTC
(In reply to Jakub Jelinek from comment #39)
> It is an ABI change, so I think it is highly undesirable to backport.  It is
> enough that people will have to rebuild many packages built by GCC 7
> prereleases, if it is backported, they would have to rebuild also anything
> built by GCC 5 or 6.

On the other hand, the initial bug report showed that the old compilers weren't even self-consistent.
Comment 41 Jakub Jelinek 2017-04-25 15:39:45 UTC
That is true, but we've got only a single bugreport about this since the 5.2 release (21 months), so it doesn't trigger that often.  For static data members the ABI is self-consistent, for some template types in templates it is indeed not if the instantiation is different before the instantiation of those templates.  By changing the ABI now in 5/6 I'm afraid we'd break far more code than is already broken.
Comment 42 Jakub Jelinek 2017-04-25 15:40:45 UTC
Oh, and we surely need to document this in gcc-7/changes.html, and I'd think we should make sure to also document the earlier ARM ABI change in gcc-5/changes.html for GCC 5.2 when it has changed and document there the change was only fixed in 7.1.
Comment 43 Richard Earnshaw 2017-04-25 16:03:24 UTC
Hmm, so how about just inserting the warning in the broken compilers?
Comment 44 Jakub Jelinek 2017-04-25 16:06:11 UTC
Works for me.  That would mean roughly applying the two patches, but instead of doing else if (res < 0) do if (res) (and something similar for aarch64).
Comment 45 Jakub Jelinek 2017-04-25 16:47:06 UTC
Author: jakub
Date: Tue Apr 25 16:46:34 2017
New Revision: 247258

URL: https://gcc.gnu.org/viewcvs?rev=247258&root=gcc&view=rev
Log:
	PR target/77728
	* config/arm/arm.c: Include gimple.h.
	(aapcs_layout_arg): Emit -Wpsabi note if arm_needs_doubleword_align
	returns negative, increment ncrn only if it returned positive.
	(arm_needs_doubleword_align): Return int instead of bool,
	ignore DECL_ALIGN of non-FIELD_DECL TYPE_FIELDS chain
	members, but if there is any such non-FIELD_DECL
	> PARM_BOUNDARY aligned decl, return -1 instead of false.
	(arm_function_arg): Emit -Wpsabi note if arm_needs_doubleword_align
	returns negative, increment nregs only if it returned positive.
	(arm_setup_incoming_varargs): Likewise.
	(arm_function_arg_boundary): Emit -Wpsabi note if
	arm_needs_doubleword_align returns negative, return
	DOUBLEWORD_ALIGNMENT only if it returned positive.
testsuite/
	* g++.dg/abi/pr77728-1.C: New test.

Added:
    trunk/gcc/testsuite/g++.dg/abi/pr77728-1.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/arm/arm.c
    trunk/gcc/testsuite/ChangeLog
Comment 46 Jakub Jelinek 2017-04-25 16:48:04 UTC
Author: jakub
Date: Tue Apr 25 16:47:32 2017
New Revision: 247259

URL: https://gcc.gnu.org/viewcvs?rev=247259&root=gcc&view=rev
Log:
	PR target/77728
	* config/arm/arm.c: Include gimple.h.
	(aapcs_layout_arg): Emit -Wpsabi note if arm_needs_doubleword_align
	returns negative, increment ncrn only if it returned positive.
	(arm_needs_doubleword_align): Return int instead of bool,
	ignore DECL_ALIGN of non-FIELD_DECL TYPE_FIELDS chain
	members, but if there is any such non-FIELD_DECL
	> PARM_BOUNDARY aligned decl, return -1 instead of false.
	(arm_function_arg): Emit -Wpsabi note if arm_needs_doubleword_align
	returns negative, increment nregs only if it returned positive.
	(arm_setup_incoming_varargs): Likewise.
	(arm_function_arg_boundary): Emit -Wpsabi note if
	arm_needs_doubleword_align returns negative, return
	DOUBLEWORD_ALIGNMENT only if it returned positive.
testsuite/
	* g++.dg/abi/pr77728-1.C: New test.

Added:
    branches/gcc-7-branch/gcc/testsuite/g++.dg/abi/pr77728-1.C
Modified:
    branches/gcc-7-branch/gcc/ChangeLog
    branches/gcc-7-branch/gcc/config/arm/arm.c
    branches/gcc-7-branch/gcc/testsuite/ChangeLog
Comment 47 Jakub Jelinek 2017-04-25 16:49:28 UTC
Fixed for 7.1+.
Comment 48 Yichao Yu 2017-04-25 16:56:46 UTC
Thanks for fixing this. I didn't follow all the comments since I'm not familiar with the C++ ABI so just to make sure I understand what's happening is it that the bug is caused by a inconsistency in C++ ABI for certain classes which can happen on both ARM and AArch64 (although not for AArch64 in this case)?

Is this now fixed for gcc 7+ for both ARM and AArch64? (Should this be closed now or only when there's a release?) And btw, when is the estimated release time of 7.1?
Comment 49 rguenther@suse.de 2017-04-25 17:17:46 UTC
On April 25, 2017 5:20:29 PM GMT+02:00, "jakub at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org> wrote:
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77728
>
>--- Comment #39 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
>It is an ABI change, so I think it is highly undesirable to backport. 
>It is
>enough that people will have to rebuild many packages built by GCC 7
>prereleases, if it is backported, they would have to rebuild also
>anything
>built by GCC 5 or 6.

Not sure if really a good option but one could back port a change just ignoring TYPE_DECLs which means making the ABI self-consistent but still incompatible with both 5.2 and 7.1 ... (Or provide configurability via -mabi-version and a default chosen at configure time...)
Comment 50 Jonathan Wakely 2017-04-25 17:32:54 UTC
(In reply to ktkachov from comment #3)
> Started with r225465.
> Something to do with alignment.
> I wonder if it's related to PR69841 ?

Seems to be the same. Maybe PR 80149 too?
Comment 51 Richard Earnshaw 2017-04-26 11:04:18 UTC
(In reply to Jonathan Wakely from comment #50)
> (In reply to ktkachov from comment #3)
> > Started with r225465.
> > Something to do with alignment.
> > I wonder if it's related to PR69841 ?
> 
> Seems to be the same. Maybe PR 80149 too?

With the latest trunk sources the testcase for PR80149 triggers the new ABI change warning; the testcase for PR69841 does also, but only when optimization is O1 or lower.
Comment 52 Richard Earnshaw 2017-04-26 11:05:09 UTC
*** Bug 80149 has been marked as a duplicate of this bug. ***
Comment 53 Richard Earnshaw 2017-04-26 11:06:50 UTC
*** Bug 69841 has been marked as a duplicate of this bug. ***
Comment 54 Jakub Jelinek 2017-04-26 11:17:41 UTC
The notices are only for ABI of actually compiled code, so it depends on optimizations, if functions are inlined or optimized away, we don't report anything, so at -O0 when fewer functions are inlined and almost nothing is optimized away you can get more -Wpsabi notices.
Comment 55 Jakub Jelinek 2017-04-27 07:13:43 UTC
Author: jakub
Date: Thu Apr 27 07:13:10 2017
New Revision: 247292

URL: https://gcc.gnu.org/viewcvs?rev=247292&root=gcc&view=rev
Log:
	PR target/77728
	* config/aarch64/aarch64.c (struct aarch64_fn_arg_alignment): Remove.
	(aarch64_function_arg_alignment): Return unsigned int again, but still
	ignore TYPE_FIELDS chain decls other than FIELD_DECLs.
	(aarch64_layout_arg): Adjust aarch64_function_arg_alignment caller.
	Don't emit -Wpsabi note.
	(aarch64_function_arg_boundary): Likewise.
	(aarch64_gimplify_va_arg_expr): Adjust aarch64_function_arg_alignment
	caller.
testsuite/
	* g++.dg/abi/pr77728-2.C: Don't expect -Wpsabi notes.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/aarch64/aarch64.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/g++.dg/abi/pr77728-2.C
Comment 56 Jakub Jelinek 2017-04-27 07:14:56 UTC
Author: jakub
Date: Thu Apr 27 07:14:24 2017
New Revision: 247293

URL: https://gcc.gnu.org/viewcvs?rev=247293&root=gcc&view=rev
Log:
	PR target/77728
	* config/aarch64/aarch64.c (struct aarch64_fn_arg_alignment): Remove.
	(aarch64_function_arg_alignment): Return unsigned int again, but still
	ignore TYPE_FIELDS chain decls other than FIELD_DECLs.
	(aarch64_layout_arg): Adjust aarch64_function_arg_alignment caller.
	Don't emit -Wpsabi note.
	(aarch64_function_arg_boundary): Likewise.
	(aarch64_gimplify_va_arg_expr): Adjust aarch64_function_arg_alignment
	caller.
testsuite/
	* g++.dg/abi/pr77728-2.C: Don't expect -Wpsabi notes.

Modified:
    branches/gcc-7-branch/gcc/ChangeLog
    branches/gcc-7-branch/gcc/config/aarch64/aarch64.c
    branches/gcc-7-branch/gcc/testsuite/ChangeLog
    branches/gcc-7-branch/gcc/testsuite/g++.dg/abi/pr77728-2.C
Comment 57 Marek Polacek 2017-05-05 15:38:35 UTC
Author: mpolacek
Date: Fri May  5 15:38:04 2017
New Revision: 247639

URL: https://gcc.gnu.org/viewcvs?rev=247639&root=gcc&view=rev
Log:
	PR target/77728
	* config/arm/arm.c: Include gimple.h.
	(aapcs_layout_arg): Emit -Wpsabi note if arm_needs_doubleword_align
	returns negative, increment ncrn if it returned non-zero.
	(arm_needs_doubleword_align): Return int instead of bool,
	ignore DECL_ALIGN of non-FIELD_DECL TYPE_FIELDS chain
	members, but if there is any such non-FIELD_DECL
	> PARM_BOUNDARY aligned decl, return -1 instead of false.
	(arm_function_arg): Emit -Wpsabi note if arm_needs_doubleword_align
	returns negative, increment nregs if it returned non-zero.
	(arm_setup_incoming_varargs): Likewise.
	(arm_function_arg_boundary): Emit -Wpsabi note if
	arm_needs_doubleword_align returns negative, return
	DOUBLEWORD_ALIGNMENT if it returned non-zero.

	* g++.dg/abi/pr77728-1.C: New test.

Added:
    branches/gcc-6-branch/gcc/testsuite/g++.dg/abi/pr77728-1.C
Modified:
    branches/gcc-6-branch/gcc/ChangeLog
    branches/gcc-6-branch/gcc/config/arm/arm.c
    branches/gcc-6-branch/gcc/testsuite/ChangeLog
Comment 58 Maxim Kuvyrkov 2017-05-10 10:48:36 UTC
Shouldn't the release note [*] also specify AArch64 as the affected target, not just ARM/AArch32?

[*] https://gcc.gnu.org/gcc-7/changes.html
Comment 59 Jakub Jelinek 2017-05-10 10:53:29 UTC
It does mention it:
"GCC has been updated to the latest revision of the procedure call standard (AAPCS64) to provide support for paramater passing when data types have been over-aligned."

There were two issues, one is that old GCC had issues with passing of overaligned and underaligned variables by value.  This was a problem in GCC < 5.2 for arm32 and GCC <= 6.x for aarch64.  And another thing was the bug mentioned here, introduced for arm32 in 5.2 and for aarch64 only during development of GCC 7.  So there was no release for aarch64 with that bug.
Comment 60 Maxim Kuvyrkov 2017-05-10 11:12:21 UTC
(In reply to Jakub Jelinek from comment #59)
> And another thing was the bug
> mentioned here, introduced for arm32 in 5.2 and for aarch64 only during
> development of GCC 7.  So there was no release for aarch64 with that bug.

Thanks Jacub, I missed the last part about aarch64 being affected only in development versions of GCC 7.