On Wed, Aug 10, 2011 at 10:48 AM, Pedro Alves <pe...@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
>
