| Message ID | 1449507714-9599-1-git-send-email-marex@denx.de |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
Hi, I haven't had time to check the previous report yet. On 07/12/15 19:01, Marek Vasut wrote: > Seems 6ae6e160 broke creating multi and script type images and even > building of mkimage itself. There are two problems with that patch. > > First is that expression (!(x == 0) || !(x == 1)) is always true for > unsigned int x. The expression must use AND (&&) not OR (||) to be > correct. It is either multi or script, so AND does not sound correct. The code should skip the following bit if either of those flags is detected. I admit I threw in the script bit as an afterthought and things went wrong there. Correct would be if( !(params.type == IH_TYPE_MULTI || params.type == IH_TYPE_SCRIPT)) I'll double-check stuff and submit a new patch > Second is the coding which causes gcc 4.9.x and newer scream gruesome > death and murder. The expression !x == 0 && !x == 1 is ambiguous and > should instead be rewritten into (x != 0) && (x != 1) to be correct. > The parenthesis are added for clarity. Weirdly enough I have gcc 4.9.2 and it did not even beep, so I don't know how it could have broken the build. Give me some time to submit a corrective patch later tonight. Cheers, Philippe
On Monday, December 07, 2015 at 06:18:02 PM, Philippe De Swert wrote: > Hi, > > I haven't had time to check the previous report yet. > > On 07/12/15 19:01, Marek Vasut wrote: > > Seems 6ae6e160 broke creating multi and script type images and even > > building of mkimage itself. There are two problems with that patch. > > > > First is that expression (!(x == 0) || !(x == 1)) is always true for > > unsigned int x. The expression must use AND (&&) not OR (||) to be > > correct. > > It is either multi or script, so AND does not sound correct. The code > should skip the following bit if either of those > flags is detected. I admit I threw in the script bit as an afterthought > and things went wrong there. > > Correct would be if( !(params.type == IH_TYPE_MULTI || params.type == > IH_TYPE_SCRIPT)) > > I'll double-check stuff and submit a new patch So yeah, !(X or Y) <=> (!X and !Y) . The patch does that. See https://en.wikipedia.org/wiki/De_Morgan%27s_laws > > Second is the coding which causes gcc 4.9.x and newer scream gruesome > > death and murder. The expression !x == 0 && !x == 1 is ambiguous and > > should instead be rewritten into (x != 0) && (x != 1) to be correct. > > The parenthesis are added for clarity. > > Weirdly enough I have gcc 4.9.2 and it did not even beep, so I don't > know how it could have broken the build. > Give me some time to submit a corrective patch later tonight. This patch should fix things. Best regards, Marek Vasut
Dear Marek, In message <1449507714-9599-1-git-send-email-marex@denx.de> you wrote: > > Second is the coding which causes gcc 4.9.x and newer scream gruesome > death and murder. The expression !x == 0 && !x == 1 is ambiguous and > should instead be rewritten into (x != 0) && (x != 1) to be correct. But (!x == 0) && (!x == 1) ist not the same as (x != 0) && (x != 1); assume x=2: (!2 == 0) && (!2 == 1) => (0 == 0) && (0 == 1) => 1 && 0 => 0 (2 != 0) && (2 != 1) => 1 && 1 => 1 ??? Best regards, Wolfgang Denk
On Mon, Dec 07, 2015 at 06:47:36PM +0100, Wolfgang Denk wrote: > Dear Marek, > > In message <1449507714-9599-1-git-send-email-marex@denx.de> you wrote: > > > > Second is the coding which causes gcc 4.9.x and newer scream gruesome > > death and murder. The expression !x == 0 && !x == 1 is ambiguous and > > should instead be rewritten into (x != 0) && (x != 1) to be correct. ok, part of the problem is that we aren't testing !x == 0 && !x == 1 (and I'm re-wording the commit msg, we had been talking about this on IRC) but "!x == 4 || !x == 6". > But (!x == 0) && (!x == 1) ist not the same as (x != 0) && (x != 1); > assume x=2: ... so this is a different thing to consider too. I'm re-wording things because in sum, what Philippe did is not straight forward, and Marek's version is.
On Monday, December 07, 2015 at 06:47:36 PM, Wolfgang Denk wrote: > Dear Marek, > > In message <1449507714-9599-1-git-send-email-marex@denx.de> you wrote: > > Second is the coding which causes gcc 4.9.x and newer scream gruesome > > death and murder. The expression !x == 0 && !x == 1 is ambiguous and > > should instead be rewritten into (x != 0) && (x != 1) to be correct. > > But (!x == 0) && (!x == 1) ist not the same as (x != 0) && (x != 1); > assume x=2: > > (!2 == 0) && (!2 == 1) => (0 == 0) && (0 == 1) => 1 && 0 => 0 > > (2 != 0) && (2 != 1) => 1 && 1 => 1 > > ??? This really depends on where you put the parenthesis and GCC complains about such ambiguous expressions. That's what the paragraph is about. I never said that ((!x) == 0) && ((!x) == 1) <=> (x != 0) && (x != 1) or equally ((!x) == 0) && ((!x) == 1) <=> !(x == 0) && !(x == 1) Best regards, Marek Vasut
On Mon, Dec 07, 2015 at 06:01:54PM +0100, Marek Vasut wrote: > Seems 6ae6e160 broke creating multi and script type images and even > building of mkimage itself. There are two problems with that patch. > > First is that expression (!(x == 0) || !(x == 1)) is always true for > unsigned int x. The expression must use AND (&&) not OR (||) to be > correct. > > Second is the coding which causes gcc 4.9.x and newer scream gruesome > death and murder. The expression !x == 0 && !x == 1 is ambiguous and > should instead be rewritten into (x != 0) && (x != 1) to be correct. > The parenthesis are added for clarity. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Tom Rini <trini@konsulko.com> > Cc: Philippe De Swert <philippedeswert@gmail.com> > Cc: Simon Glass <sjg@chromium.org> After rewording the commit message a bit (and talking with Marek on IRC about my reword), applied to u-boot/master, thanks!
Hi, On 07/12/15 21:06, Tom Rini wrote: > On Mon, Dec 07, 2015 at 06:01:54PM +0100, Marek Vasut wrote: > >> Seems 6ae6e160 broke creating multi and script type images and even >> building of mkimage itself. There are two problems with that patch. >> >> First is that expression (!(x == 0) || !(x == 1)) is always true for >> unsigned int x. The expression must use AND (&&) not OR (||) to be >> correct. >> >> After rewording the commit message a bit (and talking with Marek on IRC >> about my reword), applied to u-boot/master, thanks! >> Great! Apologies for the breakage, Philippe
diff --git a/tools/mkimage.c b/tools/mkimage.c index ae01cb1..8f8b6df 100644 --- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -311,8 +311,7 @@ NXTARG: ; exit (retval); } - if (!params.type == IH_TYPE_MULTI || - !params.type == IH_TYPE_SCRIPT) { + if ((params.type != IH_TYPE_MULTI) && (params.type != IH_TYPE_SCRIPT)) { dfd = open(params.datafile, O_RDONLY | O_BINARY); if (dfd < 0) { fprintf(stderr, "%s: Can't open %s: %s\n",
Seems 6ae6e160 broke creating multi and script type images and even building of mkimage itself. There are two problems with that patch. First is that expression (!(x == 0) || !(x == 1)) is always true for unsigned int x. The expression must use AND (&&) not OR (||) to be correct. Second is the coding which causes gcc 4.9.x and newer scream gruesome death and murder. The expression !x == 0 && !x == 1 is ambiguous and should instead be rewritten into (x != 0) && (x != 1) to be correct. The parenthesis are added for clarity. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Tom Rini <trini@konsulko.com> Cc: Philippe De Swert <philippedeswert@gmail.com> Cc: Simon Glass <sjg@chromium.org> --- tools/mkimage.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)