| Message ID | 1351979121-3769-2-git-send-email-sjg@chromium.org |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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
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
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
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
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> >
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
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
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
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
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
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);