[Bug middle-end/39954] New: [4.5 Regression] Unaligned access in gcc.dg/torture/pr26565.c

[Bug middle-end/39954] New: [4.5 Regression] Unaligned access in gcc.dg/torture/pr26565.c

[hjl dot tools at gmail dot com]

On Linux/ia64, revision 146820 gave

Apr 26 20:44:03 gnu-12 kernel: pr26565.exe(23460): unaligned access to
0x6000000000000e31, ip=0x40000000000006d0
Apr 26 20:44:03 gnu-12 kernel: pr26565.exe(23472): unaligned access to
0x6000000000000e71, ip=0x40000000000006d0
Apr 26 20:44:03 gnu-12 kernel: pr26565.exe(23484): unaligned access to
0x6000000000000e71, ip=0x40000000000006d0
Apr 26 20:44:03 gnu-12 kernel: pr26565.exe(23503): unaligned access to
0x6000000000000e71, ip=0x40000000000006d0
Apr 26 20:44:03 gnu-12 kernel: pr26565.exe(23515): unaligned access to
0x6000000000000e51, ip=0x40000000000006d0

Revision 146803 is OK.


-- 
           Summary: [4.5 Regression] Unaligned access in
                    gcc.dg/torture/pr26565.c
           Product: gcc
           Version: 4.5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: hjl dot tools at gmail dot com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Unaligned access in gcc.dg/torture/pr26565.c

[pinskia at gcc dot gnu dot org]

------- Comment #1 from pinskia at gcc dot gnu dot org  2009-04-28 21:54 -------
I think this testcase is invalid really:
    memcpy(&outdata->tv, tp, sizeof outdata->tv);

tv is packed and taking the address of a packed entry is not really a valid
thing.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Unaligned access in gcc.dg/torture/pr26565.c

[hjl dot tools at gmail dot com]

------- Comment #2 from hjl dot tools at gmail dot com  2009-04-28 21:59 -------
(In reply to comment #1)
> I think this testcase is invalid really:
>     memcpy(&outdata->tv, tp, sizeof outdata->tv);
> 
> tv is packed and taking the address of a packed entry is not really a valid
> thing.
> 

I don't believe memcpy has any alignment requirements.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Unaligned access in gcc.dg/torture/pr26565.c

[rguenth at gcc dot gnu dot org]

------- Comment #3 from rguenth at gcc dot gnu dot org  2009-04-28 22:04 -------
Indeed.  We likely fold the memcpy, but that should better preserve the
known alignment.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Unaligned access in gcc.dg/torture/pr26565.c

[pinskia at gcc dot gnu dot org]

-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.5.0


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[hjl dot tools at gmail dot com]

------- Comment #4 from hjl dot tools at gmail dot com  2009-04-28 23:43 -------
Revision 146817:

http://gcc.gnu.org/ml/gcc-cvs/2009-04/msg01459.html

is the cause.


-- 

hjl dot tools at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |matz at suse dot de
            Summary|[4.5 Regression] Unaligned  |[4.5 Regression] Revision
                   |access in                   |146817 caused unaligned
                   |gcc.dg/torture/pr26565.c    |access in
                   |                            |gcc.dg/torture/pr26565.c
   Target Milestone|4.5.0                       |---


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[pinskia at gcc dot gnu dot org]

------- Comment #5 from pinskia at gcc dot gnu dot org  2009-04-28 23:48 -------
(In reply to comment #3)
> Indeed.  We likely fold the memcpy, but that should better preserve the
> known alignment.

What known alignment?
The alignment of the type is known to be the same alignment as long inside a
struct.

I mentioned the same reasoning back in PR 26565 in the first place.  Even
without looking into that bug report I can to the same conclusion as I did
before.


-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.5.0


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[matz at gcc dot gnu dot org]

------- Comment #6 from matz at gcc dot gnu dot org  2009-04-29 00:17 -------
Andrew is right.  There is no "known" alignment (or misalignment) as far
as the gimple code is concerned, only the natural alignment:

send_probe (struct outdata * outdata, struct timeval * tp)
{
  struct timeval * D.1240;
  D.1240_2 = &outdata_1(D)->tv;
  memcpy (D.1240_2, tp_3(D), 8); [tail call]
  return;
}

Here D.1240_2 is of type "struct timeval *", hence has natural alignment
of 8 byte, so the expansion of memcpy is completely correct to assume the
alignment.

The bug is the type of D.1240.  It needs to be an unaligned version of the
above, but I don't think we have such pointer types.

It worked before expand from SSA simply by accident, because TER would
have folded the component_ref into the memcpy call, so the expander could
see what this really is about.  Had there been two such memcpy calls TER
wouldn't have done that and the same problem would have happened.

The user should have the possibility to announce the unalignedness to the
compiler via casts, like so:

  memcpy((char*)&outdata->tv, tp, sizeof outdata->tv);

(or void* or whatever).  This doesn't work currently, as the emitted gimple
code loses that cast.

So, there're two things: explicit alignment changing casts are lost and
the type of ADDR_EXPR of non-naturally aligned fields has the wrong pointer
type (losing the unalignedness).

The former problem is a bit problematic to solve, as parameter passing
always has an implicit conversion to the formal parameter type (void* in this
case).  We don't want to lose alignment info just because of that conversion,
only for explicit ones.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[hjl dot tools at gmail dot com]

------- Comment #7 from hjl dot tools at gmail dot com  2009-04-29 00:33 -------
(In reply to comment #6)
> Andrew is right.  There is no "known" alignment (or misalignment) as far
> as the gimple code is concerned, only the natural alignment:
> 
> send_probe (struct outdata * outdata, struct timeval * tp)
> {
>   struct timeval * D.1240;
>   D.1240_2 = &outdata_1(D)->tv;
>   memcpy (D.1240_2, tp_3(D), 8); [tail call]
>   return;
> }
> 

How does gimple handle

typedef int foo __attribute__ ((aligned(1)));

void
bar (foo *p, char *b)
{
  __builtin_memcpy (p, b, 30);
}


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[hjl dot tools at gmail dot com]

------- Comment #8 from hjl dot tools at gmail dot com  2009-04-29 00:42 -------
This code:

---
void *memcpy(void *dest, const void *src, __SIZE_TYPE__ n);

struct timeval {
    long tv_sec;
};

typedef struct timeval packed   __attribute__((aligned(1)));

struct outdata {
    long align;
    char seq;
    packed tv;
};

void send_probe(struct outdata *outdata, struct timeval *tp)
__attribute__((noin
line));
void send_probe(struct outdata *outdata, struct timeval *tp)
{
    memcpy(&outdata->tv, tp, sizeof outdata->tv);
}

struct timeval t;
struct outdata outdata;

int main()
{
  send_probe(&outdata, &t);
  return 0;
}
---

works fine. It seems that we mishandled __attribute__((packed)).


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[pinskia at gcc dot gnu dot org]

------- Comment #9 from pinskia at gcc dot gnu dot org  2009-04-29 00:43 -------
(In reply to comment #7)
> How does gimple handle
> 
> typedef int foo __attribute__ ((aligned(1)));

The C/C++ front-end ignores the aligned here but does not mention that to you. 
That is a different issue though, see PR 29436 and PR 38611.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[joseph at codesourcery dot com]

------- Comment #10 from joseph at codesourcery dot com  2009-04-29 11:33 -------
Subject: Re:  [4.5 Regression] Revision 146817 caused
 unaligned access in gcc.dg/torture/pr26565.c

On Wed, 29 Apr 2009, matz at gcc dot gnu dot org wrote:

> The user should have the possibility to announce the unalignedness to the
> compiler via casts, like so:
> 
>   memcpy((char*)&outdata->tv, tp, sizeof outdata->tv);
> 
> (or void* or whatever).  This doesn't work currently, as the emitted gimple
> code loses that cast.
> 
> So, there're two things: explicit alignment changing casts are lost and
> the type of ADDR_EXPR of non-naturally aligned fields has the wrong pointer
> type (losing the unalignedness).
> 
> The former problem is a bit problematic to solve, as parameter passing
> always has an implicit conversion to the formal parameter type (void* in this
> case).  We don't want to lose alignment info just because of that conversion,
> only for explicit ones.

In ISO C terms casts change alignment information in the opposite 
direction to the one you want - if the conversion sequence contains 
anywhere a pointer to an N-byte aligned type, you can assume that the 
pointer is N-byte aligned, so you take the highest alignment from the 
sequence of types, not the lowest.  The problem is representing the 
address of an unaligned field as a pointer-to-aligned-type.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[matz at gcc dot gnu dot org]

------- Comment #11 from matz at gcc dot gnu dot org  2009-04-29 11:38 -------
Thanks for the clarification.  So there indeed is only one issue, that the
temporary has an aligned type.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[mmitchel at gcc dot gnu dot org]

------- Comment #12 from mmitchel at gcc dot gnu dot org  2009-05-05 15:48 -------
Yes, we've been discussing the interaction between attributes and the type
system for at least a decade. :-)  In type-theoretic terms, the address of a
packed int has type pointer-to-packed-int, not pointer-to-int.  And the latter
can be safely converted to the former, but the former cannot be safely
converted to the latter.

Michael, unfortunately, if it was your change that introduced this regression,
you are responsible for solving the problem.  The Right Answer, as you suggest,
is to include the packed attribute in the type system, but I suspect that will
be a major effort.  Unfortunately, I don't know what else to suggest...


-- 

mmitchel at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[hjl dot tools at gmail dot com]

------- Comment #13 from hjl dot tools at gmail dot com  2009-05-05 15:59 -------
(In reply to comment #12)

> Michael, unfortunately, if it was your change that introduced this regression,
> you are responsible for solving the problem.  The Right Answer, as you suggest,
> is to include the packed attribute in the type system, but I suspect that will
> be a major effort.  Unfortunately, I don't know what else to suggest...
> 

We have the aligned attribute in the type system. Can't we implement

typedef struct timeval packed   __attribute__((packed));

as

typedef struct timeval packed   __attribute__((aligned(1)));


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[matz at gcc dot gnu dot org]

------- Comment #14 from matz at gcc dot gnu dot org  2009-05-06 09:48 -------
Mark, I'm certainly willing to help fixing this, but it's not a new
regression.  Before my change TER _sometimes_ worked around the pre-existing
problem, but it's trivial to construct a case exposing it also before
expand from SSA:

void * send_probe(struct outdata *outdata, struct timeval *tp)
{
    memcpy(&outdata->tv, tp, sizeof outdata->tv);
    return &outdata->tv;
}

(or having more than one reference to &outdata->tv in some other way).

gcc 4.3, 4.4 and trunk before expand from SSA will emit unaligned accesses
here.  I haven't checked older ones, but the problem exists since introduction
of SSA (and TER in its current form).


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[rguenth at gcc dot gnu dot org]

------- Comment #15 from rguenth at gcc dot gnu dot org  2009-05-06 11:08 -------
For optimization purposes differently aligned type variants should be treated
as a variant type, thus liked in the TYPE_NEXT_VARIANT chain (probably also
required to get correct behavior when assigning alias sets).  The gimple
type system then should treat conversions of pointer types with differing
alignment of their target types as not useless.  Eventually conversion
folding machinery needs to be adjusted, at least the STRIP_NOPS code needs to.

The question is also whether for the FIELD_DECLs we properly put in place
types with TYPE_ALIGN == 8 into packed structures.  I have no idea if we
want to set TYPE_USER_ALIGN on these types though - and what it makes for
a difference.


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu dot
                   |                            |org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[matz at gcc dot gnu dot org]

------- Comment #16 from matz at gcc dot gnu dot org  2009-05-06 11:20 -------
Joseph: I'd need some advise where best starting to fix this.  I see some
options, when we want to deal with such construct:

struct S { T member __attribute__((packed)); };
... struct S s; ... &s->member ...

(1) make the type of the FIELD_DECL not be T but an aligned(1) variant
    of it.  Currently simply DECL_PACKED is set on the FIELD_DECL, but
    that's ignored in some contextes, which results in the other options:
(2) make the type of the COMPONENT_REF "s->member" not be T but an aligned(1)
    variant of it, and
(3) make the type of the ADDR_EXPR "&s->member" not be T* but (T-aligned1)*.

It seems that option (1) would be the most thorough one as then surely no
other code can accidentally construct an aligned access to it.  I think
amending handle_packed_attribute to retroactively patch TREE_TYPE if called
on a FIELD_DECL might do it.

Option (3) I think is the least appealing one, as I'd always fear that there
might be other usages of the COMPONENT_REF than the ADDR_EXPR where the
alignment might matter.  It would also mean constructing the pointer type
not from the RHS but using some variant, which seems to introduce a conceptual
inconsistency.

Option (2) lies somewhere in between, although it should be nearly equivalent
to option 1, as there aren't many other contexts than COMPONENT_REF where
a pure FIELD_DECL could be used.  But option (2) has the advantage that the
structure type is completely laid out already, not so for option (1).  That
might be important for performance sometimes, e.g. when the user declares
a field as packed (or aligned(1)), but it so happens (due to other members)
that it got it's natural alignment in the end.  In that case we don't _have_
to construct unaligned types.


-- 

matz at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |joseph at codesourcery dot
                   |                            |com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[rguenther at suse dot de]

------- Comment #17 from rguenther at suse dot de  2009-05-06 11:45 -------
Subject: Re:  [4.5 Regression] Revision 146817 caused
 unaligned access in gcc.dg/torture/pr26565.c

On Wed, 6 May 2009, matz at gcc dot gnu dot org wrote:

> ------- Comment #16 from matz at gcc dot gnu dot org  2009-05-06 11:20 -------
> Option (2) lies somewhere in between, although it should be nearly equivalent
> to option 1, as there aren't many other contexts than COMPONENT_REF where
> a pure FIELD_DECL could be used.  But option (2) has the advantage that the
> structure type is completely laid out already, not so for option (1).  That
> might be important for performance sometimes, e.g. when the user declares
> a field as packed (or aligned(1)), but it so happens (due to other members)
> that it got it's natural alignment in the end.  In that case we don't _have_
> to construct unaligned types.

I'm all for option (1), but concering (2) - a packed structure does
have alignment 1, so how can we derive anything but alignment 1 from
(the sizes of, supposedly) other members?  That is, we would have
to build an explicitly aligned _decl_ of the packed type to be able
to derive more than 1 alignment.  No?

Richard.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[matz at gcc dot gnu dot org]

------- Comment #18 from matz at gcc dot gnu dot org  2009-05-06 11:57 -------
The structure in this testcase is not packed.  One member is.  Or are you
talking about some different case?

Let's suppose you are and you mean something like this:
  struct S {T1 m1; T2 m2; ...} __attribute__((packed));
then IMO all members should also have DECL_PACKED set.  I don't think this
is currently the case (only TYPE_PACKED is set on the whole struct type), but
conceptually that is the ultimate meaning.

If we don't do that we still can create such unaligned types when building
COMPONENT_REFs, when the base datum has TYPE_PACKED set, i.e. option (2).
But let's deal with one case after the other, I'm currently concerned with
only packed members.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[matz at gcc dot gnu dot org]

------- Comment #19 from matz at gcc dot gnu dot org  2009-05-06 20:39 -------
Something like this implements option 1.  It's probably hopelessly inefficient
(always generating a new type for each packed field), and wrong, and generally
just a bad idea, but it does fix the testcase, execute.exp still works and
bootstrap is already in the target libs :)
And it only deals with packed fields, not those having an aligned() attribute.

Index: c-common.c
===================================================================
--- c-common.c  (Revision 147187)
+++ c-common.c  (Arbeitskopie)
@@ -5813,6 +5813,18 @@ c_init_attributes (void)
 #undef DEF_ATTR_TREE_LIST
 }

+static tree
+get_aligned_type (tree t, int align)
+{
+  tree ret;
+  if (TYPE_PACKED (t))
+    return t;
+  ret = build_variant_type_copy (t);
+  TYPE_ALIGN (ret) = align * BITS_PER_UNIT;
+  TYPE_USER_ALIGN (ret) = 1;
+  return ret;
+}
+
 /* Attribute handlers common to C front ends.  */

 /* Handle a "packed" attribute; arguments as in
@@ -5837,7 +5849,10 @@ handle_packed_attribute (tree *node, tre
                 "%qE attribute ignored for field of type %qT",
                 name, TREE_TYPE (*node));
       else
-       DECL_PACKED (*node) = 1;
+       {
+         DECL_PACKED (*node) = 1;
+         TREE_TYPE (*node) = get_aligned_type (TREE_TYPE (*node), 1);
+       }
     }
   /* We can't set DECL_PACKED for a VAR_DECL, because the bit is
      used for DECL_REGISTER.  It wouldn't mean anything anyway.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[rguenther at suse dot de]

------- Comment #20 from rguenther at suse dot de  2009-05-06 21:02 -------
Subject: Re:  [4.5 Regression] Revision 146817 caused
 unaligned access in gcc.dg/torture/pr26565.c

On Wed, 6 May 2009, matz at gcc dot gnu dot org wrote:

> ------- Comment #19 from matz at gcc dot gnu dot org  2009-05-06 20:39 -------
> Something like this implements option 1.  It's probably hopelessly inefficient
> (always generating a new type for each packed field), and wrong, and generally
> just a bad idea, but it does fix the testcase, execute.exp still works and
> bootstrap is already in the target libs :)
> And it only deals with packed fields, not those having an aligned() attribute.
> 
> Index: c-common.c
> ===================================================================
> --- c-common.c  (Revision 147187)
> +++ c-common.c  (Arbeitskopie)
> @@ -5813,6 +5813,18 @@ c_init_attributes (void)
>  #undef DEF_ATTR_TREE_LIST
>  }
> 
> +static tree
> +get_aligned_type (tree t, int align)
> +{
> +  tree ret;
> +  if (TYPE_PACKED (t))
> +    return t;

Walking the type variants and searching for what we now build would
fix the inefficiency.  And of course this function needs a comment ;)

> +  ret = build_variant_type_copy (t);
> +  TYPE_ALIGN (ret) = align * BITS_PER_UNIT;
> +  TYPE_USER_ALIGN (ret) = 1;

It seems that only ever place_field looks at this flag.  How
is the effect of setting it here?

Overall I like this patch.

Richard.

> +  return ret;
> +}
> +
>  /* Attribute handlers common to C front ends.  */
> 
>  /* Handle a "packed" attribute; arguments as in
> @@ -5837,7 +5849,10 @@ handle_packed_attribute (tree *node, tre
>                  "%qE attribute ignored for field of type %qT",
>                  name, TREE_TYPE (*node));
>        else
> -       DECL_PACKED (*node) = 1;
> +       {
> +         DECL_PACKED (*node) = 1;
> +         TREE_TYPE (*node) = get_aligned_type (TREE_TYPE (*node), 1);
> +       }
>      }
>    /* We can't set DECL_PACKED for a VAR_DECL, because the bit is
>       used for DECL_REGISTER.  It wouldn't mean anything anyway.
> 
> 
> 


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[matz at suse dot de]

------- Comment #21 from matz at suse dot de  2009-05-06 21:39 -------
Subject: Re:  [4.5 Regression] Revision 146817 caused
 unaligned access in gcc.dg/torture/pr26565.c

On Wed, 6 May 2009, rguenther at suse dot de wrote:

> > +  tree ret;
> > +  if (TYPE_PACKED (t))
> > +    return t;
> 
> Walking the type variants and searching for what we now build would
> fix the inefficiency.  And of course this function needs a comment ;)

I anyway am unsure about using variants of types for this.  E.g. some 
other lookup functions when searching for a qualified type simply walk the 
list and take the first one with matching qualifiers.  Now if there 
suddenly are multiple variants with same qualifiers but different 
alignment in there it might chose an overly aligned type accidentally.  
Or maybe I'm confused, hmm...  (but yes, that's the obvious fix for the 
inefficiency).  Can qualified types themself be a new base 
(TYPE_MAIN_VARIANT) for a new chain?  In that case it would work just 
fine.

> > +  ret = build_variant_type_copy (t);
> > +  TYPE_ALIGN (ret) = align * BITS_PER_UNIT;
> > +  TYPE_USER_ALIGN (ret) = 1;
> 
> It seems that only ever place_field looks at this flag.

TYPE_USER_ALIGN?  By place_field and update_alignment_for_field, and it's 
copied into DECL_USER_ALIGN (which is used in more places).

TYPE_USER_ALIGN only ever seems to guard calls to ADJUST_FIELD_ALIGN when 
PCC_BITFIELD_TYPE_MATTERS or BITFIELD_NBYTES_LIMITED.  But I have no idea 
why a user specified alignment should not also be affected by 
ADJUST_FIELD_ALIGN.  It all seems to have to do with the general theme of 
not overriding user-specified alignment in any way that the compiler 
normally takes to derive alignments.

In any case it seems better to leave this alone, stor-layout.c is filled 
with sometimes quite arcane conditions and special cases and probably 
nobody in the world can test all combinations of strange ABIs and funny 
requirements or backward compatibilities.

> How is the effect of setting it here?

Well, the user explicitely put "attribute((packed))" there so it seems 
reasonable to deal with this as if he also specified an explicit 
alignment.

> Overall I like this patch.

Much to my surprise it even seems to work up to now, bootstrap was okay 
testsuite still running.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[rguenther at suse dot de]

------- Comment #22 from rguenther at suse dot de  2009-05-06 21:55 -------
Subject: Re:  [4.5 Regression] Revision 146817 caused
 unaligned access in gcc.dg/torture/pr26565.c

On Wed, 6 May 2009, matz at suse dot de wrote:

> ------- Comment #21 from matz at suse dot de  2009-05-06 21:39 -------
> Subject: Re:  [4.5 Regression] Revision 146817 caused
>  unaligned access in gcc.dg/torture/pr26565.c
> 
> On Wed, 6 May 2009, rguenther at suse dot de wrote:
> 
> > > +  tree ret;
> > > +  if (TYPE_PACKED (t))
> > > +    return t;
> > 
> > Walking the type variants and searching for what we now build would
> > fix the inefficiency.  And of course this function needs a comment ;)
> 
> I anyway am unsure about using variants of types for this.  E.g. some 
> other lookup functions when searching for a qualified type simply walk the 
> list and take the first one with matching qualifiers.  Now if there 
> suddenly are multiple variants with same qualifiers but different 
> alignment in there it might chose an overly aligned type accidentally.  
> Or maybe I'm confused, hmm...  (but yes, that's the obvious fix for the 
> inefficiency).  Can qualified types themself be a new base 
> (TYPE_MAIN_VARIANT) for a new chain?  In that case it would work just 
> fine.

Well, build_variant_type_copy already inserts the variant in the
list ... but yes, existing walkers would need to be changed to
also verify matching alignment to the main variant.  OTOH we
may end up creating regular variants for the different align
variants.  Ugh.

Maybe instead using build_distinct_type_copy on the main
variant and using type_hash_canon for efficiency and setting
TYPE_CANONICAL to the type with the natural alignment is better.
This may pose problems with C frontend internal compatibility
and diagnostics.

> 
> > > +  ret = build_variant_type_copy (t);
> > > +  TYPE_ALIGN (ret) = align * BITS_PER_UNIT;
> > > +  TYPE_USER_ALIGN (ret) = 1;
> > 
> > It seems that only ever place_field looks at this flag.
> 
> TYPE_USER_ALIGN?  By place_field and update_alignment_for_field, and it's 
> copied into DECL_USER_ALIGN (which is used in more places).
> 
> TYPE_USER_ALIGN only ever seems to guard calls to ADJUST_FIELD_ALIGN when 
> PCC_BITFIELD_TYPE_MATTERS or BITFIELD_NBYTES_LIMITED.  But I have no idea 
> why a user specified alignment should not also be affected by 
> ADJUST_FIELD_ALIGN.  It all seems to have to do with the general theme of 
> not overriding user-specified alignment in any way that the compiler 
> normally takes to derive alignments.
> 
> In any case it seems better to leave this alone, stor-layout.c is filled 
> with sometimes quite arcane conditions and special cases and probably 
> nobody in the world can test all combinations of strange ABIs and funny 
> requirements or backward compatibilities.
> 
> > How is the effect of setting it here?
> 
> Well, the user explicitely put "attribute((packed))" there so it seems 
> reasonable to deal with this as if he also specified an explicit 
> alignment.

Ok.

> > Overall I like this patch.
> 
> Much to my surprise it even seems to work up to now, bootstrap was okay 
> testsuite still running.

Probably making the type a variant at least makes the FE happy ;)

Richard.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[matz at gcc dot gnu dot org]

------- Comment #23 from matz at gcc dot gnu dot org  2009-05-06 21:56 -------
No regressions on x86_64-linux.  I'm baffled.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[danglin at gcc dot gnu dot org]

------- Comment #24 from danglin at gcc dot gnu dot org  2009-05-10 18:40 -------
I see this on hppa64-hp-hpux11.11.


-- 

danglin at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |danglin at gcc dot gnu dot
                   |                            |org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[sje at cup dot hp dot com]

------- Comment #25 from sje at cup dot hp dot com  2009-06-17 20:37 -------
Micheal, are you still working on this bug?  It looks like you had a patch for
it that you were testing but I don't see anything after that and the test is
still failing.


-- 

sje at cup dot hp dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2009-06-17 20:37:33
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[matz at gcc dot gnu dot org]

------- Comment #26 from matz at gcc dot gnu dot org  2009-06-18 11:26 -------
Hmm, yeah, I should probably clean up my hack and formally submit it.  But
I had hoped to get some advise from the frontend guys, because I was surprised
that the patch actually worked.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[rguenth at gcc dot gnu dot org]

------- Comment #27 from rguenth at gcc dot gnu dot org  2010-01-18 11:12 -------
Testing a patch to do minimal surgery to restore previous behavior (thus, fix
this regression but not the fundamental frontend / middle-end problem).


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |rguenth at gcc dot gnu dot
                   |dot org                     |org
             Status|NEW                         |ASSIGNED
   Last reconfirmed|2009-06-17 20:37:33         |2010-01-18 11:12:33
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[rguenth at gcc dot gnu dot org]

------- Comment #28 from rguenth at gcc dot gnu dot org  2010-01-18 11:24 -------
Btw, get_pointer_alignment should get passed an access type to put it into
context.  For alignment of say INDIRECT_REFs it would be the pointed-to type
but for function arguments in general it needs to be 'char' if you don't know
anything about the kind of the accesses the function does.  That would be
the default/fallback align get_pointer_alignment has to assume.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[rguenth at gcc dot gnu dot org]

------- Comment #29 from rguenth at gcc dot gnu dot org  2010-01-18 11:43 -------
(In reply to comment #28)
> Btw, get_pointer_alignment should get passed an access type to put it into
> context.  For alignment of say INDIRECT_REFs it would be the pointed-to type
> but for function arguments in general it needs to be 'char' if you don't know
> anything about the kind of the accesses the function does.  That would be
> the default/fallback align get_pointer_alignment has to assume.

Which, if you look at all current callers, boils down to effectively

Index: gcc/builtins.c
===================================================================
--- gcc/builtins.c      (revision 156006)
+++ gcc/builtins.c      (working copy)
@@ -369,8 +369,7 @@ get_pointer_alignment (tree exp, unsigne
   if (!POINTER_TYPE_P (TREE_TYPE (exp)))
     return 0;

-  align = TYPE_ALIGN (TREE_TYPE (TREE_TYPE (exp)));
-  align = MIN (align, max_align);
+  align = BITS_PER_UNIT;

   while (1)
     {
@@ -380,9 +379,6 @@ get_pointer_alignment (tree exp, unsigne
          exp = TREE_OPERAND (exp, 0);
          if (! POINTER_TYPE_P (TREE_TYPE (exp)))
            return align;
-
-         inner = TYPE_ALIGN (TREE_TYPE (TREE_TYPE (exp)));
-         align = MIN (inner, max_align);
          break;

        case POINTER_PLUS_EXPR:


which of course will regress generated code quite a bit (even though it's
correct).  We might be able to recover some bits by walking SSA defs
here but we'll still lose when reaching function arguments.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[rguenth at gcc dot gnu dot org]

------- Comment #30 from rguenth at gcc dot gnu dot org  2010-01-18 12:59 -------
Fixed.


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954

[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[rguenth at gcc dot gnu dot org]

------- Comment #31 from rguenth at gcc dot gnu dot org  2010-01-18 13:00 -------
Subject: Bug 39954

Author: rguenth
Date: Mon Jan 18 12:59:50 2010
New Revision: 156008

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=156008
Log:
2010-01-18  Richard Guenther  <rguenther@suse.de>

        PR middle-end/39954
        * cfgexpand.c (expand_call_stmt): TER pointer arguments in
        builtin calls.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/cfgexpand.c


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954