Patchwork [U-Boot,2/3] md5: Fix gcc-4.7 build problem in md5

login
register
mail settings
Submitter Simon Glass
Date Nov. 3, 2012, 9:45 p.m.
Message ID <1351979121-3769-2-git-send-email-sjg@chromium.org>
Download mbox | patch
Permalink /patch/196956/
State Not Applicable, archived
Headers show

Comments

Simon Glass - Nov. 3, 2012, 9:45 p.m.
From: Han Shen <shenhan@google.com>

Fixed by replacing pointer casting with memcpy.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 lib/md5.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
Wolfgang Denk - Nov. 4, 2012, 12:32 a.m.
Dear Simon Glass,

In message <1351979121-3769-2-git-send-email-sjg@chromium.org> you wrote:
> From: Han Shen <shenhan@google.com>
> 
> Fixed by replacing pointer casting with memcpy.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>  lib/md5.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/md5.c b/lib/md5.c
> index 2ae4a06..9791e59 100644
> --- a/lib/md5.c
> +++ b/lib/md5.c
> @@ -153,8 +153,7 @@ MD5Final(unsigned char digest[16], struct MD5Context *ctx)
>  	byteReverse(ctx->in, 14);
>  
>  	/* Append length in bits and transform */
> -	ctx->in32[14] = ctx->bits[0];
> -	ctx->in32[15] = ctx->bits[1];
> +	memcpy(ctx->in + 14 * sizeof(__u32), ctx->bits, 2 * sizeof(__u32));

This makes the code actually unreadable.  Please add at least a
comment what this is doing.

Best regards,

Wolfgang Denk
Wolfgang Denk - Nov. 4, 2012, 12:39 a.m.
Dear Simon,

In message <20121104003242.92729200056@gemini.denx.de> I wrote:
> 
> >  	/* Append length in bits and transform */
> > -	ctx->in32[14] = ctx->bits[0];
> > -	ctx->in32[15] = ctx->bits[1];
> > +	memcpy(ctx->in + 14 * sizeof(__u32), ctx->bits, 2 * sizeof(__u32));
> 
> This makes the code actually unreadable.  Please add at least a
> comment what this is doing.

Actually I think this shoul dbe split into two memcpy commands, using
the addresses of the respective array elements directly, without such
manual pointer arithmetics.

Best regards,

Wolfgang Denk
Pavel Machek - Nov. 5, 2012, 8:03 p.m.
Hi!

> In message <20121104003242.92729200056@gemini.denx.de> I wrote:
> > 
> > >  	/* Append length in bits and transform */
> > > -	ctx->in32[14] = ctx->bits[0];
> > > -	ctx->in32[15] = ctx->bits[1];
> > > +	memcpy(ctx->in + 14 * sizeof(__u32), ctx->bits, 2 * sizeof(__u32));
> > 
> > This makes the code actually unreadable.  Please add at least a
> > comment what this is doing.
> 
> Actually I think this shoul dbe split into two memcpy commands, using
> the addresses of the respective array elements directly, without such
> manual pointer arithmetics.

I guess bigger question is: why does gcc miscompile that, and is it
guaranteed that it will not miscompile the memcpy?

Is compiler barrier somewhere better solution?
									Pavel
Wolfgang Denk - Nov. 5, 2012, 8:50 p.m.
Dear Pavel,

In message <20121105200340.GA15821@xo-6d-61-c0.localdomain> you wrote:
> 
> > > >  	/* Append length in bits and transform */
> > > > -	ctx->in32[14] = ctx->bits[0];
> > > > -	ctx->in32[15] = ctx->bits[1];
> > > > +	memcpy(ctx->in + 14 * sizeof(__u32), ctx->bits, 2 * sizeof(__u32));
> > > 
> > > This makes the code actually unreadable.  Please add at least a
> > > comment what this is doing.
> > 
> > Actually I think this shoul dbe split into two memcpy commands, using
> > the addresses of the respective array elements directly, without such
> > manual pointer arithmetics.
> 
> I guess bigger question is: why does gcc miscompile that, and is it
> guaranteed that it will not miscompile the memcpy?

I did not see Simon mentioning anythin about incorrect compilation.
My understanding was that it's just the usual "dereferencing
type-punned pointer" warnings issue.


Simon, what was the actual problem this was supposed to fix?

Best regards,

Wolfgang Denk
Han Shen - Nov. 5, 2012, 9:15 p.m.
On Mon, Nov 5, 2012 at 12:50 PM, Wolfgang Denk <wd@denx.de> wrote:

> Dear Pavel,
>
> In message <20121105200340.GA15821@xo-6d-61-c0.localdomain> you wrote:
> >
> > > > >         /* Append length in bits and transform */
> > > > > -       ctx->in32[14] = ctx->bits[0];
> > > > > -       ctx->in32[15] = ctx->bits[1];
> > > > > +       memcpy(ctx->in + 14 * sizeof(__u32), ctx->bits, 2 *
> sizeof(__u32));
> > > >
> > > > This makes the code actually unreadable.  Please add at least a
> > > > comment what this is doing.
> > >
> > > Actually I think this shoul dbe split into two memcpy commands, using
> > > the addresses of the respective array elements directly, without such
> > > manual pointer arithmetics.
> >
> > I guess bigger question is: why does gcc miscompile that, and is it
> > guaranteed that it will not miscompile the memcpy?
>
> I did not see Simon mentioning anythin about incorrect compilation.
> My understanding was that it's just the usual "dereferencing
> type-punned pointer" warnings issue.
>

Yup, it's a compilation warning. issue.  (but warnings being treated as
errors.)


>
>
> Simon, what was the actual problem this was supposed to fix?
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> Let's say the docs present a simplified view of reality...    :-)
>                       - Larry Wall in  <6940@jpl-devvax.JPL.NASA.GOV>
>
Pavel Machek - Nov. 5, 2012, 10:51 p.m.
Hi!
 
    In message <20121105200340.GA15821@xo-6d-61-c0.localdomain> you wrote:
>    >
>    > > > >         /* Append length in bits and transform */
>    > > > > -       ctx->in32[14] = ctx->bits[0];
>    > > > > -       ctx->in32[15] = ctx->bits[1];
>    > > > > +       memcpy(ctx->in + 14 * sizeof(__u32), ctx->bits, 2 *
>    sizeof(__u32));
>    > > >
>    > > > This makes the code actually unreadable.  Please add at least a
>    > > > comment what this is doing.
>    > >
>    > > Actually I think this shoul dbe split into two memcpy commands,
>    using
>    > > the addresses of the respective array elements directly, without
>    such
>    > > manual pointer arithmetics.
>    >
>    > I guess bigger question is: why does gcc miscompile that, and is it
>    > guaranteed that it will not miscompile the memcpy?
> 
>      I did not see Simon mentioning anythin about incorrect compilation.
>      My understanding was that it's just the usual "dereferencing
>      type-punned pointer" warnings issue.


Is there some alternate solution? The memcpy is really ugly...

Plus... does it solve the issue? The code does not look like being
compatible with strict pointer aliasing... and I don't think memcpy()
helps.

arch/nds32/config.mk:PLATFORM_RELFLAGS  += -fno-strict-aliasing
-fno-common -mrelax
arch/x86/config.mk:PLATFORM_CPPFLAGS += -fno-strict-aliasing

We should really do that globally.
								Pavel
Marek Vasut - Nov. 6, 2012, 12:56 a.m.
Dear Pavel Machek,

> Hi!
> 
>     In message <20121105200340.GA15821@xo-6d-61-c0.localdomain> you wrote:
> >    > > > >         /* Append length in bits and transform */
> >    > > > > 
> >    > > > > -       ctx->in32[14] = ctx->bits[0];
> >    > > > > -       ctx->in32[15] = ctx->bits[1];
> >    > > > > +       memcpy(ctx->in + 14 * sizeof(__u32), ctx->bits, 2 *
> >    
> >    sizeof(__u32));
> >    
> >    > > > This makes the code actually unreadable.  Please add at least a
> >    > > > comment what this is doing.
> >    > > 
> >    > > Actually I think this shoul dbe split into two memcpy commands,
> >    
> >    using
> >    
> >    > > the addresses of the respective array elements directly, without
> >    
> >    such
> >    
> >    > > manual pointer arithmetics.
> >    > 
> >    > I guess bigger question is: why does gcc miscompile that, and is it
> >    > guaranteed that it will not miscompile the memcpy?
> >    > 
> >      I did not see Simon mentioning anythin about incorrect compilation.
> >      My understanding was that it's just the usual "dereferencing
> >      type-punned pointer" warnings issue.
> 
> Is there some alternate solution? The memcpy is really ugly...
> 
> Plus... does it solve the issue? The code does not look like being
> compatible with strict pointer aliasing... and I don't think memcpy()
> helps.
> 
> arch/nds32/config.mk:PLATFORM_RELFLAGS  += -fno-strict-aliasing
> -fno-common -mrelax
> arch/x86/config.mk:PLATFORM_CPPFLAGS += -fno-strict-aliasing
> 
> We should really do that globally.
> 								Pavel

Were you even able to replicate this problem in the first place? Isn't this 
whole "problem" a problem of a broken (ubuntu/linaro) toolchain again?

Best regards,
Marek Vasut
Pavel Machek - Nov. 6, 2012, 11:07 a.m.
On Tue 2012-11-06 01:56:50, Marek Vasut wrote:
> Dear Pavel Machek,
> 
> > Hi!
> > 
> >     In message <20121105200340.GA15821@xo-6d-61-c0.localdomain> you wrote:
> > >    > > > >         /* Append length in bits and transform */
> > >    > > > > 
> > >    > > > > -       ctx->in32[14] = ctx->bits[0];
> > >    > > > > -       ctx->in32[15] = ctx->bits[1];
> > >    > > > > +       memcpy(ctx->in + 14 * sizeof(__u32), ctx->bits, 2 *
> > >    
> > >    sizeof(__u32));
> > Is there some alternate solution? The memcpy is really ugly...
> > 
> > Plus... does it solve the issue? The code does not look like being
> > compatible with strict pointer aliasing... and I don't think memcpy()
> > helps.
> > 
> > arch/nds32/config.mk:PLATFORM_RELFLAGS  += -fno-strict-aliasing
> > -fno-common -mrelax
> > arch/x86/config.mk:PLATFORM_CPPFLAGS += -fno-strict-aliasing
> > 
> > We should really do that globally.
> 
> Were you even able to replicate this problem in the first place? Isn't this 
> whole "problem" a problem of a broken (ubuntu/linaro) toolchain again?

This is not something you can replicate. At least md5 code is unsafe
with strict aliasing, probably most of u-boot, because low-level
people write code like that. Thus we should do
-fno-strict-aliasing. Otherwise compiler may decide in future to
"miscompile" our code, even if it compiles it correctly now.

									Pavel
Marek Vasut - Nov. 6, 2012, 10:30 p.m.
Dear Pavel Machek,

> On Tue 2012-11-06 01:56:50, Marek Vasut wrote:
> > Dear Pavel Machek,
> > 
> > > Hi!
> > > 
> > >     In message <20121105200340.GA15821@xo-6d-61-c0.localdomain> you wrote:
> > > >    > > > >         /* Append length in bits and transform */
> > > >    > > > > 
> > > >    > > > > -       ctx->in32[14] = ctx->bits[0];
> > > >    > > > > -       ctx->in32[15] = ctx->bits[1];
> > > >    > > > > +       memcpy(ctx->in + 14 * sizeof(__u32), ctx->bits, 2
> > > >    > > > > *
> > > >    
> > > >    sizeof(__u32));
> > > 
> > > Is there some alternate solution? The memcpy is really ugly...
> > > 
> > > Plus... does it solve the issue? The code does not look like being
> > > compatible with strict pointer aliasing... and I don't think memcpy()
> > > helps.
> > > 
> > > arch/nds32/config.mk:PLATFORM_RELFLAGS  += -fno-strict-aliasing
> > > -fno-common -mrelax
> > > arch/x86/config.mk:PLATFORM_CPPFLAGS += -fno-strict-aliasing
> > > 
> > > We should really do that globally.
> > 
> > Were you even able to replicate this problem in the first place? Isn't
> > this whole "problem" a problem of a broken (ubuntu/linaro) toolchain
> > again?
> 
> This is not something you can replicate. At least md5 code is unsafe
> with strict aliasing, probably most of u-boot, because low-level
> people write code like that. Thus we should do
> -fno-strict-aliasing. Otherwise compiler may decide in future to
> "miscompile" our code, even if it compiles it correctly now.
> 
> 									Pavel

I dont think -fno-strict-aliasing is the way to go, it'll only hide the problem, 
no?

Best regards,
Marek Vasut
Simon Glass - Nov. 7, 2012, 10:18 p.m.
Hi,

On Tue, Nov 6, 2012 at 2:30 PM, Marek Vasut <marex@denx.de> wrote:
> Dear Pavel Machek,
>
>> On Tue 2012-11-06 01:56:50, Marek Vasut wrote:
>> > Dear Pavel Machek,
>> >
>> > > Hi!
>> > >
>> > >     In message <20121105200340.GA15821@xo-6d-61-c0.localdomain> you wrote:
>> > > >    > > > >         /* Append length in bits and transform */
>> > > >    > > > >
>> > > >    > > > > -       ctx->in32[14] = ctx->bits[0];
>> > > >    > > > > -       ctx->in32[15] = ctx->bits[1];
>> > > >    > > > > +       memcpy(ctx->in + 14 * sizeof(__u32), ctx->bits, 2
>> > > >    > > > > *
>> > > >
>> > > >    sizeof(__u32));
>> > >
>> > > Is there some alternate solution? The memcpy is really ugly...
>> > >
>> > > Plus... does it solve the issue? The code does not look like being
>> > > compatible with strict pointer aliasing... and I don't think memcpy()
>> > > helps.
>> > >
>> > > arch/nds32/config.mk:PLATFORM_RELFLAGS  += -fno-strict-aliasing
>> > > -fno-common -mrelax
>> > > arch/x86/config.mk:PLATFORM_CPPFLAGS += -fno-strict-aliasing
>> > >
>> > > We should really do that globally.
>> >
>> > Were you even able to replicate this problem in the first place? Isn't
>> > this whole "problem" a problem of a broken (ubuntu/linaro) toolchain
>> > again?
>>
>> This is not something you can replicate. At least md5 code is unsafe
>> with strict aliasing, probably most of u-boot, because low-level
>> people write code like that. Thus we should do
>> -fno-strict-aliasing. Otherwise compiler may decide in future to
>> "miscompile" our code, even if it compiles it correctly now.
>>
>>                                                                       Pavel
>
> I dont think -fno-strict-aliasing is the way to go, it'll only hide the problem,
> no?

Yes I agree.

I believe this problem might have been a mistake on my part. I think I
incorrectly merged Marek's change 'b68d63c GCC47: Fix warning in
md5.c' when I was testing. Or it could me that my compiler was broken,
as for a while it was generating these errors when it should not.

So I believe we can drop this patch as I am no longer seeing the warning.

If so, sorry for the noise.

Regards,
Simon

>
> Best regards,
> Marek Vasut
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

Patch

diff --git a/lib/md5.c b/lib/md5.c
index 2ae4a06..9791e59 100644
--- a/lib/md5.c
+++ b/lib/md5.c
@@ -153,8 +153,7 @@  MD5Final(unsigned char digest[16], struct MD5Context *ctx)
 	byteReverse(ctx->in, 14);
 
 	/* Append length in bits and transform */
-	ctx->in32[14] = ctx->bits[0];
-	ctx->in32[15] = ctx->bits[1];
+	memcpy(ctx->in + 14 * sizeof(__u32), ctx->bits, 2 * sizeof(__u32));
 
 	MD5Transform(ctx->buf, (__u32 *) ctx->in);
 	byteReverse((unsigned char *) ctx->buf, 4);