Re: [patch, ia64] Fix unaligned accesses on IA64 from dwarf2out.c
On 08/10/2011 09:52 AM, Steve Ellcey wrote: > * md5.c (md5_read_ctx): Handle mis-aligned resbuf pointer. Ok. r~
Re: [patch, ia64] Fix unaligned accesses on IA64 from dwarf2out.c
On Wed, 2011-08-10 at 11:19 +0200, Richard Guenther wrote:
> On Wed, Aug 10, 2011 at 10:48 AM, Pedro Alves wrote:
> >
> > 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.
I like this idea. How about this patch. I am still testing it but it
should work.
Steve Ellcey
s...@cup.hp.com
2011-08-10 Steve Ellcey
* md5.c (md5_read_ctx): Handle mis-aligned resbuf pointer.
Index: md5.c
===
--- md5.c (revision 177411)
+++ md5.c (working copy)
@@ -76,15 +76,19 @@ md5_init_ctx (struct md5_ctx *ctx)
/* Put result from CTX in first 16 bytes following RESBUF. The result
must be in little endian byte order.
- IMPORTANT: On some systems it is required that RESBUF is correctly
- aligned for a 32 bits value. */
+ IMPORTANT: RESBUF may not be aligned as strongly as MD5_UNIT32 so we
+ put things in a local (aligned) buffer first, then memcpy into RESBUF. */
void *
md5_read_ctx (const struct md5_ctx *ctx, void *resbuf)
{
- ((md5_uint32 *) resbuf)[0] = SWAP (ctx->A);
- ((md5_uint32 *) resbuf)[1] = SWAP (ctx->B);
- ((md5_uint32 *) resbuf)[2] = SWAP (ctx->C);
- ((md5_uint32 *) resbuf)[3] = SWAP (ctx->D);
+ md5_unit32 buffer[4];
+
+ buffer[0] = SWAP (ctx->A);
+ buffer[1] = SWAP (ctx->B);
+ buffer[2] = SWAP (ctx->C);
+ buffer[3] = SWAP (ctx->D);
+
+ memcpy (resbuf, buffer, 16);
return resbuf;
}
Re: [patch, ia64] Fix unaligned accesses on IA64 from dwarf2out.c
On Wed, Aug 10, 2011 at 10:48 AM, Pedro Alves 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
>
Re: [patch, ia64] Fix unaligned accesses on IA64 from dwarf2out.c
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).
--
Pedro Alves
Re: [patch, ia64] Fix unaligned accesses on IA64 from dwarf2out.c
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
s...@cup.hp.com
Re: [patch, ia64] Fix unaligned accesses on IA64 from dwarf2out.c
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~
Re: [patch, ia64] Fix unaligned accesses on IA64 from dwarf2out.c
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 s...@cup.hp.com
Re: [patch, ia64] Fix unaligned accesses on IA64 from dwarf2out.c
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~
Re: [patch, ia64] Fix unaligned accesses on IA64 from dwarf2out.c
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
s...@cup.hp.com
Re: [patch, ia64] Fix unaligned accesses on IA64 from dwarf2out.c
On 08/08/2011 02:24 PM, Steve Ellcey wrote: > Do you mean a union, as Andreas suggested? No, I mean struct. The union is wrong, because both data elements are live at the same time. r~
Re: [patch, ia64] Fix unaligned accesses on IA64 from dwarf2out.c
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~
Do you mean a union, as Andreas suggested? What do you think of this
patch? I used a union of the unsigned char array that we need and a
dummy variable of type md5_uint32 since md5_read_ctx is treating the
pointer as a pointer to an array of type md5_uint32 and so that is the
alignment that we need.
If this is considered the right approach, we can make a similar change
to fold-const.c which is why I put the definition of md5_resbuf in md5.h
instead of in dwarf2out.c.
Steve Ellcey
s...@cup.hp.com
2011-08-08 Steve Ellcey
* md5.h (md5_resbuf): New union type.
gcc/ChangeLog
2011-08-08 Steve Ellcey
* dwarf2out.c (generate_type_signature): Change type of checksum to
union and change usages to access resbuf field.
(compute_section_prefix): Ditto.
(optimize_macinfo_range): Ditto.
Index: include/md5.h
===
--- include/md5.h (revision 177411)
+++ include/md5.h (working copy)
@@ -89,6 +89,15 @@ struct md5_ctx
char buffer[128] ATTRIBUTE_ALIGNED_ALIGNOF(md5_uint32);
};
+/* Union for buffer data that must be aligned when accessed by
+ md5_read_ctx (usually called from md5_finish_ctx). */
+
+union md5_resbuf
+{
+ md5_uint32 dummy;
+ unsigned char resbuf[16];
+};
+
/*
* The following three functions are build up the low level used in
* the functions `md5_stream' and `md5_buffer'.
Index: gcc/dwarf2out.c
===
--- gcc/dwarf2out.c (revision 177411)
+++ gcc/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;
+ union md5_resbuf checksum;
dw_die_ref decl;
name = get_AT_string (die, DW_AT_name);
@@ -6390,9 +6390,9 @@ generate_type_signature (dw_die_ref die,
md5_process_bytes (&die->die_tag, sizeof (die->die_tag), &ctx);
md5_process_bytes (name, strlen (name) + 1, &ctx);
- md5_finish_ctx (&ctx, checksum);
+ md5_finish_ctx (&ctx, checksum.resbuf);
- add_AT_data8 (type_node->root_die, DW_AT_GNU_odr_signature,
&checksum[8]);
+ add_AT_data8 (type_node->root_die, DW_AT_GNU_odr_signature,
&checksum.resbuf[8]);
}
/* Next, compute the complete type signature. */
@@ -6408,11 +6408,11 @@ generate_type_signature (dw_die_ref die,
/* Checksum the DIE and its children. */
die_checksum_ordered (die, &ctx, &mark);
unmark_all_dies (die);
- md5_finish_ctx (&ctx, checksum);
+ md5_finish_ctx (&ctx, checksum.resbuf);
/* Store the signature in the type node and link the type DIE and the
type node together. */
- memcpy (type_node->signature, &checksum[16 - DWARF_TYPE_SIGNATURE_SIZE],
+ memcpy (type_node->signature, &checksum.resbuf[16 -
DWARF_TYPE_SIGNATURE_SIZE],
DWARF_TYPE_SIGNATURE_SIZE);
die->die_id.die_type_node = type_node;
type_node->type_die = die;
@@ -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;
+ union md5_resbuf checksum;
/* Compute the checksum of the DIE, then append part of it as hex digits to
the name filename of the unit. */
@@ -6612,7 +6612,7 @@ compute_section_prefix (dw_die_ref unit_
mark = 0;
die_checksum (unit_die, &ctx, &mark);
unmark_all_dies (unit_die);
- md5_finish_ctx (&ctx, checksum);
+ md5_finish_ctx (&ctx, checksum.resbuf);
sprintf (name, "%s.", base);
clean_symbol_name (name);
@@ -6620,7 +6620,7 @@ compute_section_prefix (dw_die_ref unit_
p = name + strlen (name);
for (i = 0; i < 4; i++)
{
- sprintf (p, "%.2x", checksum[i]);
+ sprintf (p, "%.2x", checksum.resbuf[i]);
p += 2;
}
@@ -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;
+ union md5_resbuf checksum;
char *grp_name, *tail;
const char *base;
unsigned int i, count, encoded_filename_len, linebuf_len;
@@ -20702,7 +20702,7 @@ optimize_macinfo_range (unsigned int idx
checksum_uleb128 (cur->lineno, &ctx);
md5_process_bytes (cur->info, strlen (cur->info) + 1, &ctx);
}
- md5_finish_ctx (&ctx, checksum);
+ md5_finish_ctx (&ctx, checksum.resbuf);
count = i - idx;
Re: [patch, ia64] Fix unaligned accesses on IA64 from dwarf2out.c
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~
Re: [patch, ia64] Fix unaligned accesses on IA64 from dwarf2out.c
Steve Ellcey 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. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different."
[patch, ia64] Fix unaligned accesses on IA64 from dwarf2out.c
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
s...@cup.hp.com
2011-08-05 Steve Ellcey
* dwarf2out.c (generate_type_signature): Change decl order to force
alignment.
(compute_section_prefix):
(optimize_macinfo_range): Ditto.
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;
