| Message ID | 201108051754.p75HsR004415@catbert.cup.hp.com |
|---|---|
| State | New |
| Headers | show |
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.
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~
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
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~
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
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~
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
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).
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 >
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;