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
Confirmed the exception on GCC 5 and later
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).
Started with r225465. Something to do with alignment. I wonder if it's related to PR69841 ?
Ping. Anything I can help with debugging this?
Ping again? Anything new or I can help with here?
Anything new here?
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.
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.
(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).
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).
(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.
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?
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 ...
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.
(In reply to rguenther@suse.de from comment #13) > IMHO TYPE_DECL shouldn't even _have_ DECL_ALIGN ... Agreed.
(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
Created attachment 41237 [details] AArch64 prototype. Patch.
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 =.
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.
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?
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.
(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.
Created attachment 41254 [details] AArch64 patch .. Most review comments addressed but missed the min / max in aarch64_function_arg_boundary. Tests cleanly.
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?
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.
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.
(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.
The + else + warn_align = MAX (warn_align, DECL_ALIGN (field)); last line here is misindented, should be below se.
Also, the aarch64_function_arg_alignment and arm_needs_doubleword_align function comments need updating.
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 ""
(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.
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.
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.
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.
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).
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
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
Is there a plan to backport this fix to the 5 and 6 branches?
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.
(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.
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.
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.
Hmm, so how about just inserting the warning in the broken compilers?
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).
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
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
Fixed for 7.1+.
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?
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...)
(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?
(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.
*** Bug 80149 has been marked as a duplicate of this bug. ***
*** Bug 69841 has been marked as a duplicate of this bug. ***
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.
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
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
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
Shouldn't the release note [*] also specify AArch64 as the affected target, not just ARM/AArch32? [*] https://gcc.gnu.org/gcc-7/changes.html
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.
(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.
*** Bug 80236 has been marked as a duplicate of this bug. ***
GCC 5 branch has been closed, should be fixed in GCC 6.4 and later.
A annoyance with the fix in GCC7 is that it produces a lot of warnings from the STL I can do very little about. As we are aiming for a warning-free build I would like to supress these warnings. The problem is, that i cannot specifically supress this warning without suppressing all warning about ABI breaks for every other reason too. Would it be useful to add a compiler switch for this problem specifically?
This patch fixes a segfault seen when attaching to a process on Solaris. The steps leading to the segfault are: http://www.compilatori.com/ - procfs_target::attach calls do_attach, at this point the inferior's process slot in the target stack is empty. - do_attach adds a thread with `add http://www.acpirateradio.co.uk/ _thread (&the_procfs_target, ptid)` - in add_thread_silent, the passed target (&the_procfs_target) is passed to find_inferior_ptid http://www.logoarts.co.uk/ - find_inferior_ptid returns nullptr, as there is no inferior with this ptid that has &the_procfs_target as its process target http://www.slipstone.co.uk/ - the nullptr `inf` is passed to find_thread_ptid, which dereferences it, causing a segfault - back in procfs_target::attach, after do_attach, we push the http://embermanchester.uk/ the_procfs_target on the inferior's target stack, although we never reach this because the segfault happens before. http://connstr.net/ To fix this, I think we need to do the same as is done in inf_ptrace_target::attach: push the target early and unpush it in case the attach fails (and keep it if the attach succeeds). http://joerg.li/ Implement it by moving target_unpush_up to target.h, so it can be re-used here. Make procfs_target::attach use it. Note that just like is mentioned http://www.jopspeech.com/ in inf_ptrace_target::attach, we should push the target before calling target_pid_to_str, so that calling target_pid_to_str ends up in procfs_target::pid_to_str. http://www.wearelondonmade.com/ Tested by trying to attach on a process on gcc211 on the gcc compile farm. https://waytowhatsnext.com/ gdb/ChangeLog: This patch fixes a segfault seen when attaching to a process on Solaris. The steps leading to the segfault are: http://www.iu-bloomington.com/ - procfs_target::attach calls do_attach, at this point the inferior's process slot in the target stack is empty. https://komiya-dental.com/ - do_attach adds a thread with `add_thread (&the_procfs_target, ptid)` - in add_thread_silent, the passed target (&the_procfs_target) is passed to find_inferior_ptid http://www-look-4.com/ - find_inferior_ptid returns nullptr, as there is no inferior with this ptid that has &the_procfs_target as its process target - the nullptr `inf` is passed to find_thread_ptid, which dereferences it, causing a segfault https://www.webb-dev.co.uk/ - back in procfs_target::attach, after do_attach, we push the the_procfs_target on the inferior's target stack, although we never reach this because the segfault happens before. To fix this, I think we need to do the same as is done in inf_ptrace_target::attach: push the target early and unpush it in case the attach fails (and keep it if the attach succeeds).