Bug 80334 - [5 Regression] Segfault when taking address of copy of unaligned struct
Summary: [5 Regression] Segfault when taking address of copy of unaligned struct
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 7.0
: P2 normal
Target Milestone: 5.5
Assignee: Richard Biener
URL:
Keywords: wrong-code
Depends on:
Blocks: 87054
  Show dependency treegraph
 
Reported: 2017-04-05 21:29 UTC by Jason Rhinelander
Modified: 2022-03-30 14:06 UTC (History)
4 users (show)

See Also:
Host:
Target: x86_64-*-*, i?86-*-*
Build:
Known to work: 5.4.1, 6.3.1, 7.0.1
Known to fail: 5.4.0, 6.3.0
Last reconfirmed: 2017-04-06 00:00:00


Attachments
candidate patch to fix the error noted in comment 11 (312 bytes, patch)
2018-08-11 19:35 UTC, Alexandre Oliva
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jason Rhinelander 2017-04-05 21:29:59 UTC
I am getting a segfault with g++ 7 when trying to copy an unaligned struct into an aligned variable when the struct contains a member with alignment greater than 8 (on my amd64 architecture).

I boiled my code down into the following simplified program which exhibits the segfault under a recent g++ 7 snapshot (requires compiling with -O2 or above to trigger):

test.cpp
=====
#include <iostream>

struct A { alignas(16) char c; };
struct B { A unpacked; char d; } __attribute__((packed));

int main() {
    std::cout << "sizeof(A) = " << sizeof(A) << ", sizeof(B) = " << sizeof(B) << "\n";
    alignas(16) B b[3];

    for (int i = 0; i < 3; i++) b[i].unpacked.c = 'a' + i;

    for (int i = 0; i < 3; i++) {
        std::cout << "i=" << i << "; copying..." << std::endl;
        auto a = new A(b[i].unpacked);
        std::cout << "copied value = " << a->c << std::endl;
    }
}
=====



If I change the `alignas(16)` on the member in `struct A` to `alignas(8)` or `alignas(4)` there is no segfault; there also is no segfault under -O0 or -O1, or under g++ 6.

(The `alignas(16) char` was a `long double` in the original code, which has alignof == 16).

The alignas(16) on the array in main is just there to force alignment on the first element of `b`: with that alignment, the *first* copy succeeds because the `unpacked` member happens to be correctly aligned; the call in the second iteration of the loop (when the member isn't aligned) triggers the segfault.
Comment 1 Martin Liška 2017-04-06 07:28:56 UTC
Confirmed, started with r235518.
Comment 2 Jakub Jelinek 2017-04-06 09:20:49 UTC
Testcase without STL headers:
struct A { alignas(16) char c; };
struct B { A unpacked; char d; } __attribute__((packed));

int
main()
{
  alignas(16) B b[3];
  for (int i = 0; i < 3; i++) b[i].unpacked.c = 'a' + i;
  for (int i = 0; i < 3; i++) {
    auto a = new A(b[i].unpacked);
    __builtin_printf ("%c\n", a->c);
  }
}

Not really sure if this is valid testcase though, by placing it into an array you are violating the alignments of the fields in second and following elements.
If you want to align just the start of the array, put the alignas on the array, not on the types.

At expansion (and likely already in GIMPLE) we are assuming 128-bit alignment of b[i].unpacked):
(mem:TI (reg:DI 105 [ ivtmp.9 ]) [1 MEM[base: _21, offset: 0B]+0 S16 A128]))
so this is not STV's pass fault, just that when using vector insns the target behaves as strict alignment rather than forgiving all alignment violations.

If you use
struct A { long long c; };
struct B { A unpacked; char d; } __attribute__((packed));

int
main()
{
  B b[3];
  for (int i = 0; i < 3; i++) b[i].unpacked.c = 'a' + i;
  for (int i = 0; i < 3; i++) {
    auto a = new A(b[i].unpacked);
    __builtin_printf ("%c\n", a->c);
  }
}
then we assume 64-bit alignment on the b[i].unpacked read too.

With
struct A { char c __attribute__((aligned (16))); };
struct B { A unpacked; char d; } __attribute__((packed));

int
main()
{
  B b[3] __attribute__((aligned (16)));
  for (int i = 0; i < 3; i++) b[i].unpacked.c = 'a' + i;
  for (int i = 0; i < 3; i++) {
    A *a = new A(b[i].unpacked);
    __builtin_printf ("%c\n", a->c);
  }
}
and -O2 I can track the using of A128 in the read from b[i].unpacked to r162001,
and r161703 still emits there A8.
r161703 in *.optimized still has:
  D.2157_35 = (void *) ivtmp.10_27;
  *a_11 = MEM[base: D.2157_35];
and in *.vregs has:
(insn 17 16 18 4 pr80334-3.C:10 (set (reg:DI 69)
        (mem/s:DI (reg:DI 63 [ ivtmp.10 ]) [2 b[i].unpacked+0 S8 A8])) 61 {*movdi_internal_rex64} (nil))
No idea where it discovered the b[i].unpacked!
r162001 has in *.optimized:
  D.2157_35 = (void *) ivtmp.10_27;
  *a_11 = MEM[(struct B[3] *)D.2157_35];
and in *.vregs:
(insn 17 16 18 4 pr80334-3.C:10 (set (reg:DI 69)
        (mem/s:DI (reg:DI 63 [ ivtmp.10 ]) [3 MEM[(struct B[3] *)D.2157_35]+0 S8 A128])) 61 {*movdi_internal_rex64} (nil))

Unfortunately various revisions between those 2 (don't have that many) just hang on the testcase, so can't bisect it exactly.
Comment 3 Richard Biener 2017-04-06 09:36:42 UTC
Fix:

Index: gcc/tree-ssa-loop-ivopts.c
===================================================================
--- gcc/tree-ssa-loop-ivopts.c  (revision 246724)
+++ gcc/tree-ssa-loop-ivopts.c  (working copy)
@@ -7396,7 +7396,11 @@ rewrite_use_address (struct ivopts_data
     base_hint = var_at_stmt (data->current_loop, cand, use->stmt);
 
   iv = var_at_stmt (data->current_loop, cand, use->stmt);
-  ref = create_mem_ref (&bsi, TREE_TYPE (*use->op_p), &aff,
+  tree type = TREE_TYPE (*use->op_p);
+  unsigned int align = get_object_alignment (*use->op_p);
+  if (align != TYPE_ALIGN (type))
+    type = build_aligned_type (type, align);
+  ref = create_mem_ref (&bsi, type, &aff,
                        reference_alias_ptr_type (*use->op_p),
                        iv, base_hint, data->speed);
   copy_ref_info (ref, *use->op_p);
Comment 4 Richard Biener 2017-04-06 13:57:07 UTC
Author: rguenth
Date: Thu Apr  6 13:56:35 2017
New Revision: 246731

URL: https://gcc.gnu.org/viewcvs?rev=246731&root=gcc&view=rev
Log:
2017-04-06  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/80334
	* tree-ssa-loop-ivopts.c (rewrite_use_address): Properly
	preserve alignment of accesses.

	* g++.dg/torture/pr80334.C: New testcase.

Added:
    trunk/gcc/testsuite/g++.dg/torture/pr80334.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-ssa-loop-ivopts.c
Comment 5 Richard Biener 2017-04-06 14:01:04 UTC
Fixed on trunk sofar, queued for backporting (I think the issue is latent).
Comment 6 Jason Rhinelander 2017-04-06 15:26:51 UTC
Confirming that this solves the original (unsimplified) issue for me with current trunk.
Comment 7 Richard Biener 2017-04-07 08:48:21 UTC
Author: rguenth
Date: Fri Apr  7 08:47:43 2017
New Revision: 246757

URL: https://gcc.gnu.org/viewcvs?rev=246757&root=gcc&view=rev
Log:
2017-04-07  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/80334
	* g++.dg/torture/pr80334.C: Use __BIGGEST_ALIGNMENT__ for
	alignas on stack.

Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/g++.dg/torture/pr80334.C
Comment 8 Richard Biener 2017-05-09 12:27:58 UTC
Author: rguenth
Date: Tue May  9 12:27:24 2017
New Revision: 247790

URL: https://gcc.gnu.org/viewcvs?rev=247790&root=gcc&view=rev
Log:
2017-05-09  Richard Biener  <rguenther@suse.de>

	Backport from mainline
	2017-03-28  Richard Biener  <rguenther@suse.de>

	PR middle-end/80222
	* gimple-fold.c (gimple_fold_indirect_ref): Do not touch
	TYPE_REF_CAN_ALIAS_ALL references.
	* fold-const.c (fold_indirect_ref_1): Likewise.

	* g++.dg/pr80222.C: New testcase.

	2017-04-06  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/80262
	* tree-sra.c (build_ref_for_offset): Preserve address-space
	information.
	* tree-ssa-sccvn.c (vn_reference_maybe_forwprop_address):
	Drop useless address-space information on MEM_REF offsets.

	* gcc.target/i386/pr80262.c: New testcase.

	2017-04-03  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/80275
	* fold-const.c (split_address_to_core_and_offset): Handle
	POINTER_PLUS_EXPR.

	* g++.dg/opt/pr80275.C: New testcase.

	2017-04-06  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/80334
	* tree-ssa-loop-ivopts.c (rewrite_use_address): Properly
	preserve alignment of accesses.

	* g++.dg/torture/pr80334.C: New testcase.

	2017-04-10  Richard Biener  <rguenther@suse.de>

	PR middle-end/80362
	* fold-const.c (fold_binary_loc): Look at unstripped ops when
	looking for NEGATE_EXPR in -A / -B to A / B folding.

	* gcc.dg/torture/pr80362.c: New testcase.

	2017-04-25  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/80492
	* alias.c (compare_base_decls): Handle registers with asm
	specification conservatively.

	* gcc.dg/pr80492.c: New testcase.

	2017-04-27  Richard Biener  <rguenther@suse.de>

	PR middle-end/80539
	* tree-chrec.c (chrec_fold_plus_poly_poly): Deal with not
	being in loop-closed SSA form conservatively.
	(chrec_fold_multiply_poly_poly): Likewise.

	* gcc.dg/torture/pr80539.c: New testcase.

Added:
    branches/gcc-6-branch/gcc/testsuite/g++.dg/opt/pr80275.C
    branches/gcc-6-branch/gcc/testsuite/g++.dg/pr80222.C
    branches/gcc-6-branch/gcc/testsuite/g++.dg/torture/pr80334.C
    branches/gcc-6-branch/gcc/testsuite/gcc.dg/pr80492.c
    branches/gcc-6-branch/gcc/testsuite/gcc.dg/torture/pr80362.c
    branches/gcc-6-branch/gcc/testsuite/gcc.dg/torture/pr80539.c
    branches/gcc-6-branch/gcc/testsuite/gcc.target/i386/pr80262.c
Modified:
    branches/gcc-6-branch/gcc/ChangeLog
    branches/gcc-6-branch/gcc/alias.c
    branches/gcc-6-branch/gcc/fold-const.c
    branches/gcc-6-branch/gcc/gimple-fold.c
    branches/gcc-6-branch/gcc/testsuite/ChangeLog
    branches/gcc-6-branch/gcc/tree-chrec.c
    branches/gcc-6-branch/gcc/tree-sra.c
    branches/gcc-6-branch/gcc/tree-ssa-loop-ivopts.c
    branches/gcc-6-branch/gcc/tree-ssa-sccvn.c
Comment 9 Richard Biener 2017-05-10 07:54:18 UTC
Author: rguenth
Date: Wed May 10 07:53:45 2017
New Revision: 247827

URL: https://gcc.gnu.org/viewcvs?rev=247827&root=gcc&view=rev
Log:
2017-05-10  Richard Biener  <rguenther@suse.de>

	Backport from mainline
	2017-03-21  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/80122
	* tree-inline.c (copy_bb): Do not expans va-arg packs or
	va_arg_pack_len when the inlined call stmt requires pack
	expansion itself.
	* tree-inline.h (struct copy_body_data): Make call_stmt a gcall *.

	* gcc.dg/torture/pr80122.c: New testcase.

	2017-03-28  Richard Biener  <rguenther@suse.de>

	PR middle-end/80222
	* gimple-fold.c (gimple_fold_indirect_ref): Do not touch
	TYPE_REF_CAN_ALIAS_ALL references.
	* fold-const.c (fold_indirect_ref_1): Likewise.

	* g++.dg/pr80222.C: New testcase.

	2017-04-06  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/80334
	* tree-ssa-loop-ivopts.c (rewrite_use_address): Properly
	preserve alignment of accesses.

	* g++.dg/torture/pr80334.C: New testcase.

	2017-04-27  Richard Biener  <rguenther@suse.de>

	PR middle-end/80539
	* tree-chrec.c (chrec_fold_plus_poly_poly): Deal with not
	being in loop-closed SSA form conservatively.
	(chrec_fold_multiply_poly_poly): Likewise.

	* gcc.dg/torture/pr80539.c: New testcase.

Added:
    branches/gcc-5-branch/gcc/testsuite/g++.dg/pr80222.C
    branches/gcc-5-branch/gcc/testsuite/g++.dg/torture/pr80334.C
    branches/gcc-5-branch/gcc/testsuite/gcc.dg/torture/pr80122.c
    branches/gcc-5-branch/gcc/testsuite/gcc.dg/torture/pr80539.c
Modified:
    branches/gcc-5-branch/gcc/ChangeLog
    branches/gcc-5-branch/gcc/fold-const.c
    branches/gcc-5-branch/gcc/gimple-fold.c
    branches/gcc-5-branch/gcc/testsuite/ChangeLog
    branches/gcc-5-branch/gcc/tree-chrec.c
    branches/gcc-5-branch/gcc/tree-inline.c
    branches/gcc-5-branch/gcc/tree-inline.h
    branches/gcc-5-branch/gcc/tree-ssa-loop-ivopts.c
Comment 10 Richard Biener 2017-05-10 07:54:29 UTC
Fixed.
Comment 11 Alexandre Oliva 2018-08-11 19:32:09 UTC
FWIW, this testcase still fails with unaligned accesses in the trunk:

#ifndef T
# define T long
#endif
#ifndef R
# define R "r"
#endif


typedef T A; // #define T to long or __int128
struct B { char d; A c; } __attribute__((packed));
B b[50]; // many elements to avoid loop unrolling

int main () {
  for (int i = 0; i < sizeof(b) / sizeof(*b); i++) {
    asm ("" : "+" R (b[i].unpacked)); // #define R to "r" on ppc or "x" on x86_64
  }
}
Comment 12 Alexandre Oliva 2018-08-11 19:35:58 UTC
Created attachment 44527 [details]
candidate patch to fix the error noted in comment 11

This patch fixes the unaligned accesses in the testcase in comment 11.  I haven't yet tested it otherwise.
Comment 13 Richard Biener 2018-08-17 10:59:53 UTC
(In reply to Alexandre Oliva from comment #12)
> Created attachment 44527 [details]
> candidate patch to fix the error noted in comment 11
> 
> This patch fixes the unaligned accesses in the testcase in comment 11.  I
> haven't yet tested it otherwise.

The patch doesn't make much sense.  Whoever is "consuming" the pointer
does the wrong thing.

Please open a new bug as well.
Comment 14 Alexandre Oliva 2018-08-22 05:48:44 UTC
Done, bug 87054.

The patch in comment 13 fails libstdc++-v3; the language-independent get_object_alignment can't deal with unresolved template expressions, so using the same logic you added to ivopts to adjust the type of the compiler-introduced pointer won't do in build_fold_addr_expr_loc.
Comment 15 GCC Commits 2022-03-30 14:06:39 UTC
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:6a777ceb0e975f0efc823d2d82e676346f068151

commit r12-7920-g6a777ceb0e975f0efc823d2d82e676346f068151
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Wed Mar 30 16:04:52 2022 +0200

    testsuite: Change pr80334.C testcase to dg-do compile [PR102772]
    
    The testcase has UB at runtime, placement new shouldn't construct
    an object with certain alignment requirements into an unaligned buffer.
    
    2022-03-30  Jakub Jelinek  <jakub@redhat.com>
    
            PR tree-optimization/80334
            PR target/102772
            * g++.dg/torture/pr80334.C: Change from dg-do run to dg-do compile.