Something that's really easy to test is printf("%lu\n", (unsigned long)-1);
This is what I ran into today.This gives an unmapped memory error because of an incorrect multiply (which is how itoa is implemented). This example works with the patch provided. I think you could come up with many other examples. I think it's the upper-order bit calculation that's wrong. So unsigned multiplies near 2^64 give wrong answers. Other examples are hard to write since printf causes a crash. Jason On Mon, May 15, 2017 at 4:38 PM Gabe Black <[email protected]> wrote: > I'm a bit suspicious of this new version and how it has fixed width masks > and extra terms in the addition at the end. Could you please give me an > example of where the original code got the wrong answer? > > Gabe > > On Mon, May 15, 2017 at 1:09 PM, Jason Lowe-Power <[email protected]> > wrote: > >> We should definitely look into the statetrace tool. I'll put it on my >> giant gem5 list of things to do. >> >> Those of you in industry: There must be some test harnesses for hardware >> and internal simulators. How do you test to make sure instructions are >> implemented correctly? >> >> We've thought about using llvm or gcc tests, but they don't really test >> what we need, IIRC. >> >> Maybe this would be a good summer project for an intern: hint, hint. >> >> Jason >> >> On Mon, May 15, 2017 at 3:04 PM Gross, Joe <[email protected]> wrote: >> >>> Probably we all agree that x86 could use some automated error checking, >>> but I’m not sure of any way to do this without a ton of work. CheckerCPU >>> seemed good, but as Gabe points out, this may not be comparing against the >>> host hardware. >>> >>> >>> >>> Jason, do you know any way that’s partially or mostly implemented that >>> we could reuse? >>> >>> >>> >>> Joe >>> >>> >>> >>> *From:* Jason Lowe-Power [mailto:[email protected]] >>> *Sent:* Monday, May 15, 2017 2:54 PM >>> *To:* Gabe Black <[email protected]>; gem5 Developer List < >>> [email protected]> >>> *Cc:* Gutierrez, Anthony <[email protected]>; Gross, Joe < >>> [email protected]> >>> *Subject:* Re: [gem5-dev] Review Request 3800: x86: fix Mul1u >>> instruction >>> >>> >>> >>> Gabe: Do you want me to wait to push this patch? We can have Tony >>> re-post to gerrit if you want to review it. That would be way easier for me >>> anyway. >>> >>> >>> >>> We need a way to test for these kinds of errors. It's quite upsetting >>> that we've had this error in multiply for so long. >>> >>> >>> >>> Jason >>> >>> >>> >>> On Mon, May 15, 2017 at 2:50 PM Gabe Black <[email protected]> wrote: >>> >>> Please give me a chance to look at it. I think the checker CPU is for >>> comparing, say, the O3 CPU against what's effectively the simple CPU. If an >>> instruction doesn't do the right thing, it will do the wrong thing no >>> matter which CPU is executing it. You can compare against actual execution >>> on a real CPU using the statetrace tool, although I saw that somebody >>> rearranged all the process startup code which I'd originally carefully >>> matched against actual Linux process startup, so that tool probably doesn't >>> give any useful information any more. >>> >>> >>> >>> Gabe >>> >>> >>> >>> On Mon, May 15, 2017 at 12:45 PM, Joe Gross <[email protected]> >>> wrote: >>> >>> >>> >>> > On Feb. 4, 2017, 10:21 a.m., Jason Lowe-Power wrote: >>> > > Lol, that code is hard to understand. But, LGTM. >>> > > >>> > > What are you using to test this? Any chance you can commit the test >>> so we don't accidentally break this again in the future? >>> > >>> > Tony Gutierrez wrote: >>> > This bug was manifesting in the ROCr runtime while it is loading >>> libraries, it manifested as a segfualt (unmapped addr panic) because an >>> address calculation was corrupted due to this instruction. I manually >>> tested this instruction by comparing its output to the result of 64b >>> multiplication using __uint128_t. This is a perfect example of where >>> instruction tests would be useful. It only seemed to give bad output with >>> certain inputs, which gave the error the appearance of being >>> non-deterministic. >>> > >>> > Jason Lowe-Power wrote: >>> > Resurecting this from the grave... does anyone have any objections >>> to me just pushing this? I spent all day tracking down a bug, which was >>> fixed by this patch. >>> >>> This is an important patch for x86, I say push it. Also it would be nice >>> if we were eventually able to use the CheckerCpu for x86 (I believe it's >>> ARM-only right now), as there might be more hard-to-find bugs like this one. >>> >>> >>> - Joe >>> >>> >>> >>> ----------------------------------------------------------- >>> >>> >>> This is an automatically generated e-mail. To reply, visit: >>> >>> http://reviews.gem5.org/r/3800/#review9400 >>> ----------------------------------------------------------- >>> >>> >>> On Feb. 3, 2017, 6:13 p.m., Tony Gutierrez wrote: >>> > >>> > ----------------------------------------------------------- >>> >>> >>> > This is an automatically generated e-mail. To reply, visit: >>> > http://reviews.gem5.org/r/3800/ >>> >>> > ----------------------------------------------------------- >>> > >>> > (Updated Feb. 3, 2017, 6:13 p.m.) >>> >>> > >>> > >>> > Review request for Default. >>> > >>> > >>> > Repository: gem5 >>> > >>> > >>> > Description >>> > ------- >>> >>> >>> > >>> > Changeset 11778:c76b78110490 >>> > --------------------------- >>> > x86: fix Mul1u instruction >>> > >>> > the Mul1uFlags and Mul1u instructions perform >>> > the 64b multiplication using only 64b registers, however the >>> > method used causes the high 64b to be corrupted for certain >>> > inputs. here we fix the computation. >>> > >>> > >>> > Diffs >>> > ----- >>> > >>> > src/arch/x86/isa/microops/regop.isa >>> ed89cb178ecd7586296d2a2e83595174474db554 >>> > >>> > Diff: http://reviews.gem5.org/r/3800/diff/ >>> > >>> > >>> > Testing >>> > ------- >>> > >>> > >>> > Thanks, >>> > >>> > Tony Gutierrez >>> > >>> > >>> >>> _______________________________________________ >>> gem5-dev mailing list >>> [email protected] >>> http://m5sim.org/mailman/listinfo/gem5-dev >>> >>> > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
