Re: [U-Boot] [PATCH 2/3] md5: Fix gcc-4.7 build problem in md5
Hi, On Tue, Nov 6, 2012 at 2:30 PM, Marek Vasut ma...@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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] md5: Fix gcc-4.7 build problem in md5
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 -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] md5: Fix gcc-4.7 build problem in md5
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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] md5: Fix gcc-4.7 build problem in md5
Hi! In message 20121104003242.92729200...@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 -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] md5: Fix gcc-4.7 build problem in md5
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 -- 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: w...@denx.de Let's say the docs present a simplified view of reality...:-) - Larry Wall in 6...@jpl-devvax.jpl.nasa.gov ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] md5: Fix gcc-4.7 build problem in md5
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 -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] md5: Fix gcc-4.7 build problem in md5
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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] md5: Fix gcc-4.7 build problem in md5
On Mon, Nov 5, 2012 at 12:50 PM, Wolfgang Denk w...@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: w...@denx.de Let's say the docs present a simplified view of reality...:-) - Larry Wall in 6...@jpl-devvax.jpl.nasa.gov -- Han Shen | Software Engineer | shen...@google.com | +1-650-440-3330 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] md5: Fix gcc-4.7 build problem in md5
Dear Simon Glass, In message 1351979121-3769-2-git-send-email-...@chromium.org you wrote: From: Han Shen shen...@google.com Fixed by replacing pointer casting with memcpy. Signed-off-by: Simon Glass s...@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 -- 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: w...@denx.de Of course there's no reason for it, it's just our policy. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] md5: Fix gcc-4.7 build problem in md5
Dear Simon, In message 20121104003242.92729200...@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 -- 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: w...@denx.de The number of Unix installations has grown to 10, with more expected.- The Unix Programmer's Manual, 2nd Edition, June, 1972 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
