Patchwork [ia64] Fix unaligned accesses on IA64 from dwarf2out.c

login
register
mail settings
Submitter Steve Ellcey
Date Aug. 5, 2011, 5:54 p.m.
Message ID <201108051754.p75HsR004415@catbert.cup.hp.com>
Download mbox | patch
Permalink /patch/108705/
State New
Headers show

Comments

Steve Ellcey - Aug. 5, 2011, 5:54 p.m.
Some recent changes in dwarf2out.c have caused GCC to generate unaligned
data traps on IA64 Linux.  In looking a this I tracked it down to
md5_read_ctx in libiberty, which is called by md5_finish_ctx, which in
turn is called by various routines in dwarf2out.c.

md5_read_ctx has a comment that the buffer must be 32 bit aligned but
there is nothing in dwarf2out.c to enforce this alignment to the
checksum buffer passed in to md5_finish_ctx.  I looked at calls to
md5_finish_ctx in fold-const.c to see why these didn't seem to have a
problem and noticed that the buffers used were always declared after the
md5_ctx structure and I think that 'accidentaly' got everything aligned
correctly.  If I do the same thing on the three buffer declarations in
dwarf2out.c the misaligned messages go away and things work fine.

Obviously this isn't a perfect fix, it is relying on how GCC is laying
out local variables which isn't gauranteed, but it fixes the problem and
I thought I would see if I could get approval for this simple fix or if
people think we need a more complete fix.  If we need a better fix, what
should I do?  It doesn't look like we use the alignment attribute in the
GCC code anywhere.  I could change the type of the checksum buffer from an
array of unsigned char to something that has a stronger alignment
requirement, but that would probably require a number of casts be added
where the checksum variable is used.

So, can I get someone to approve this simple, if imperfect, patch?
Tested on IA64 Linux.

Steve Ellcey
sje@cup.hp.com


2011-08-05  Steve Ellcey  <sje@cup.hp.com>

	* dwarf2out.c (generate_type_signature): Change decl order to force
	alignment.
	(compute_section_prefix): 
	(optimize_macinfo_range): Ditto.
Andreas Schwab - Aug. 5, 2011, 6:45 p.m.
Steve Ellcey <sje@catbert.cup.hp.com> writes:

> Obviously this isn't a perfect fix, it is relying on how GCC is laying
> out local variables which isn't gauranteed, but it fixes the problem and
> I thought I would see if I could get approval for this simple fix or if
> people think we need a more complete fix.

Why not making it a union?

Andreas.
Richard Henderson - Aug. 5, 2011, 6:45 p.m.
On 08/05/2011 10:54 AM, Steve Ellcey wrote:
> -  unsigned char checksum[16];
>    struct md5_ctx ctx;
> +  unsigned char checksum[16];

How about

  struct md5_data
  {
    struct md5_ctx ctx;
    unsigned char checksum[16];
  };

  struct md5_data md5;

with the structure definition somewhere interesting?
If no where else, just dwarf2out.c file scope.


r~
Steve Ellcey - Aug. 8, 2011, 10:22 p.m.
On Fri, 2011-08-05 at 11:45 -0700, Richard Henderson wrote:
> On 08/05/2011 10:54 AM, Steve Ellcey wrote:
> > -  unsigned char checksum[16];
> >    struct md5_ctx ctx;
> > +  unsigned char checksum[16];
> 
> How about
> 
>   struct md5_data
>   {
>     struct md5_ctx ctx;
>     unsigned char checksum[16];
>   };
> 
>   struct md5_data md5;
> 
> with the structure definition somewhere interesting?
> If no where else, just dwarf2out.c file scope.
> 
> 
> r~

Oh, so after I declare md5, I call md5_finish_ctx like: 

	md5_finish_ctx (&md5.ctx, md5.checksum);

Is that what you are proposing?  It seems a bit odd to put checksum in a
a structure with ctx just to guarantee its alignment and not to pass
them around as one entity, but I guess it's no worse then using a union.

Steve Ellcey
sje@cup.hp.com
Richard Henderson - Aug. 9, 2011, 3:25 p.m.
On 08/08/2011 03:22 PM, Steve Ellcey wrote:
> Oh, so after I declare md5, I call md5_finish_ctx like: 
> 
> 	md5_finish_ctx (&md5.ctx, md5.checksum);
> 
> Is that what you are proposing?  It seems a bit odd to put checksum in a
> a structure with ctx just to guarantee its alignment and not to pass
> them around as one entity, but I guess it's no worse then using a union.

Yes, that's what I'm proposing.


r~
Steve Ellcey - Aug. 9, 2011, 9:32 p.m.
On Tue, 2011-08-09 at 08:25 -0700, Richard Henderson wrote:
> On 08/08/2011 03:22 PM, Steve Ellcey wrote:
> > Oh, so after I declare md5, I call md5_finish_ctx like: 
> > 
> > 	md5_finish_ctx (&md5.ctx, md5.checksum);
> > 
> > Is that what you are proposing?  It seems a bit odd to put checksum in a
> > a structure with ctx just to guarantee its alignment and not to pass
> > them around as one entity, but I guess it's no worse then using a union.
> 
> Yes, that's what I'm proposing.
> 
> 
> r~

I think I like using a union to ensure the alignment of checksum better.
In dwarf2out.c we are always using one md5_ctx structure and one
checksum buffer but in fold-const.c there are routines where we use one
md5_ctx structure with 4 (fold_build2_stat_loc) or 6
(fold_build3_stat_loc) different checksum buffers.

If I use a structure containing one md5_ctx struct and one checksum
array then I need to create a lot of extra md5_ctx structures in
fold-const.c.  If I use the md5_ctx as it currently is and just change
checksum from a structure to a union in order to guarantee its alignment
then I don't need to increase the space the fold-const routines are
using.

Steve Ellcey
sje@cup.hp.com
Richard Henderson - Aug. 9, 2011, 11:50 p.m.
On 08/09/2011 02:32 PM, Steve Ellcey wrote:
> On Tue, 2011-08-09 at 08:25 -0700, Richard Henderson wrote:
>> On 08/08/2011 03:22 PM, Steve Ellcey wrote:
>>> Oh, so after I declare md5, I call md5_finish_ctx like: 
>>>
>>> 	md5_finish_ctx (&md5.ctx, md5.checksum);
>>>
>>> Is that what you are proposing?  It seems a bit odd to put checksum in a
>>> a structure with ctx just to guarantee its alignment and not to pass
>>> them around as one entity, but I guess it's no worse then using a union.
>>
>> Yes, that's what I'm proposing.
>>
>>
>> r~
> 
> I think I like using a union to ensure the alignment of checksum better.
> In dwarf2out.c we are always using one md5_ctx structure and one
> checksum buffer but in fold-const.c there are routines where we use one
> md5_ctx structure with 4 (fold_build2_stat_loc) or 6
> (fold_build3_stat_loc) different checksum buffers.

I'm not keen on this.

Yes, it does happen to work, but only accidentally.  The CTX
object and the CHECKSUM object have overlapping lifetimes
within md5_read_ctx.


r~
Steve Ellcey - Aug. 10, 2011, 12:02 a.m.
On Tue, 2011-08-09 at 16:50 -0700, Richard Henderson wrote:
> > 
> > I think I like using a union to ensure the alignment of checksum better.
> > In dwarf2out.c we are always using one md5_ctx structure and one
> > checksum buffer but in fold-const.c there are routines where we use one
> > md5_ctx structure with 4 (fold_build2_stat_loc) or 6
> > (fold_build3_stat_loc) different checksum buffers.
> 
> I'm not keen on this.
> 
> Yes, it does happen to work, but only accidentally.  The CTX
> object and the CHECKSUM object have overlapping lifetimes
> within md5_read_ctx.
> 
> 
> r~

I am not proposing we put the CTX object and the CHECKSUM object into
one union.  I am proposing we leave the CTX object alone and put the
CHECKSUM object in a union with a dummy variable of type md5_uint32 in
order to ensure that the CHECKSUM object has the correct alignment.

So the usage would be:

union md5_resbuf
{
  md5_uint32 dummy;                     /* Unused variable used to ensure alignment */
  unsigned char resbuf[16];             /* The buffer we are using in md5_finish_ctx */
};

struct md5_ctx ctx;                     /* Same as before */
union md5_resbuf checksum;              /* Instead of unsigned char checksum[16]; */
md5_finish_ctx (&ctx, checksum.resbuf); /* instead of md5_finish_ctx (&ctx, checksum); */

Steve Ellcey
sje@cup.hp.com
Pedro Alves - Aug. 10, 2011, 8:48 a.m.
On Wednesday 10 August 2011 01:02:50, Steve Ellcey wrote:
> On Tue, 2011-08-09 at 16:50 -0700, Richard Henderson wrote:
> > > 
> > > I think I like using a union to ensure the alignment of checksum better.
> > > In dwarf2out.c we are always using one md5_ctx structure and one
> > > checksum buffer but in fold-const.c there are routines where we use one
> > > md5_ctx structure with 4 (fold_build2_stat_loc) or 6
> > > (fold_build3_stat_loc) different checksum buffers.
> > 
> > I'm not keen on this.
> > 
> > Yes, it does happen to work, but only accidentally.  The CTX
> > object and the CHECKSUM object have overlapping lifetimes
> > within md5_read_ctx.
> > 
> > 
> > r~
> 
> I am not proposing we put the CTX object and the CHECKSUM object into
> one union.  I am proposing we leave the CTX object alone and put the
> CHECKSUM object in a union with a dummy variable of type md5_uint32 in
> order to ensure that the CHECKSUM object has the correct alignment.
> 
> So the usage would be:
> 
> union md5_resbuf
> {
>   md5_uint32 dummy;                     /* Unused variable used to ensure alignment */
>   unsigned char resbuf[16];             /* The buffer we are using in md5_finish_ctx */
> };

Or even

union md5_resbuf
{
  md5_uint32 ui32[4];
  unsigned char ui8[16];
};

and put it in include/md5.h instead, so that all clients
can use it instead of cooking up the same themselves.
liberty's md5.c itself internaly could be make to use the
union instead of the casts?

*looks for users of md5*

but then, gold does:

  unsigned char* ov = of->get_output_view(this->build_id_note_->offset(),
                                          this->build_id_note_->data_size());

  const char* style = parameters->options().build_id();
  if (strcmp(style, "sha1") == 0)
    {
      sha1_ctx ctx;
      sha1_init_ctx(&ctx);
      sha1_process_bytes(iv, this->output_file_size_, &ctx);
      sha1_finish_ctx(&ctx, ov);
    }
  else if (strcmp(style, "md5") == 0)
    {
      md5_ctx ctx;
      md5_init_ctx(&ctx);
      md5_process_bytes(iv, this->output_file_size_, &ctx);
      md5_finish_ctx(&ctx, ov);

which makes me wonder if the right fix isn't to change
libiberty internally to not rely on the alignment.

libiberty's sha1 routines seem to have the same issue
(though they don't appear used in gcc.  gold uses them, as
seen above).
Richard Guenther - Aug. 10, 2011, 9:19 a.m.
On Wed, Aug 10, 2011 at 10:48 AM, Pedro Alves <pedro@codesourcery.com> wrote:
> On Wednesday 10 August 2011 01:02:50, Steve Ellcey wrote:
>> On Tue, 2011-08-09 at 16:50 -0700, Richard Henderson wrote:
>> > >
>> > > I think I like using a union to ensure the alignment of checksum better.
>> > > In dwarf2out.c we are always using one md5_ctx structure and one
>> > > checksum buffer but in fold-const.c there are routines where we use one
>> > > md5_ctx structure with 4 (fold_build2_stat_loc) or 6
>> > > (fold_build3_stat_loc) different checksum buffers.
>> >
>> > I'm not keen on this.
>> >
>> > Yes, it does happen to work, but only accidentally.  The CTX
>> > object and the CHECKSUM object have overlapping lifetimes
>> > within md5_read_ctx.
>> >
>> >
>> > r~
>>
>> I am not proposing we put the CTX object and the CHECKSUM object into
>> one union.  I am proposing we leave the CTX object alone and put the
>> CHECKSUM object in a union with a dummy variable of type md5_uint32 in
>> order to ensure that the CHECKSUM object has the correct alignment.
>>
>> So the usage would be:
>>
>> union md5_resbuf
>> {
>>   md5_uint32 dummy;                     /* Unused variable used to ensure alignment */
>>   unsigned char resbuf[16];             /* The buffer we are using in md5_finish_ctx */
>> };
>
> Or even
>
> union md5_resbuf
> {
>  md5_uint32 ui32[4];
>  unsigned char ui8[16];
> };
>
> and put it in include/md5.h instead, so that all clients
> can use it instead of cooking up the same themselves.
> liberty's md5.c itself internaly could be make to use the
> union instead of the casts?
>
> *looks for users of md5*
>
> but then, gold does:
>
>  unsigned char* ov = of->get_output_view(this->build_id_note_->offset(),
>                                          this->build_id_note_->data_size());
>
>  const char* style = parameters->options().build_id();
>  if (strcmp(style, "sha1") == 0)
>    {
>      sha1_ctx ctx;
>      sha1_init_ctx(&ctx);
>      sha1_process_bytes(iv, this->output_file_size_, &ctx);
>      sha1_finish_ctx(&ctx, ov);
>    }
>  else if (strcmp(style, "md5") == 0)
>    {
>      md5_ctx ctx;
>      md5_init_ctx(&ctx);
>      md5_process_bytes(iv, this->output_file_size_, &ctx);
>      md5_finish_ctx(&ctx, ov);
>
> which makes me wonder if the right fix isn't to change
> libiberty internally to not rely on the alignment.

I think that would be the best fix.  It hardly can be a performance
critical part - libiberty could use a local aligned buffer for
compute and do a copy on return.

Richard.

> libiberty's sha1 routines seem to have the same issue
> (though they don't appear used in gcc.  gold uses them, as
> seen above).
>
> --
> Pedro Alves
>

Patch

Index: dwarf2out.c
===================================================================
--- dwarf2out.c	(revision 177422)
+++ dwarf2out.c	(working copy)
@@ -6369,8 +6369,8 @@  generate_type_signature (dw_die_ref die,
 {
   int mark;
   const char *name;
-  unsigned char checksum[16];
   struct md5_ctx ctx;
+  unsigned char checksum[16];
   dw_die_ref decl;
 
   name = get_AT_string (die, DW_AT_name);
@@ -6602,8 +6602,8 @@  compute_section_prefix (dw_die_ref unit_
   char *name = XALLOCAVEC (char, strlen (base) + 64);
   char *p;
   int i, mark;
-  unsigned char checksum[16];
   struct md5_ctx ctx;
+  unsigned char checksum[16];
 
   /* Compute the checksum of the DIE, then append part of it as hex digits to
      the name filename of the unit.  */
@@ -20662,8 +20662,8 @@  optimize_macinfo_range (unsigned int idx
 {
   macinfo_entry *first, *second, *cur, *inc;
   char linebuf[sizeof (HOST_WIDE_INT) * 3 + 1];
-  unsigned char checksum[16];
   struct md5_ctx ctx;
+  unsigned char checksum[16];
   char *grp_name, *tail;
   const char *base;
   unsigned int i, count, encoded_filename_len, linebuf_len;