----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3690/#review9072 -----------------------------------------------------------
Thanks for fixing this. The only technical issue I have with this patch is that it uses bit fields, which have slightly undefined semantics. I would like to avoid that if possible. I think it would be fine to just add a uitn8_t array instead after se. A non-technical issue is that we should make sure this is included in the upstream library and then pulled into ext. Could you post a pull request to https://github.com/andysan/libfputils ? ext/fputils/include/fputils/fptypes.h (lines 71 - 72) <http://reviews.gem5.org/r/3690/#comment7810> I would prefer it if we could avoid bit annotation here. The C spec seems to specify that: "The order of allocation of bit-fields within a unit (high-order to low-order or low-order to high-order) is implementation-defined." A better solution would probably be to add a 6 byt uint8_t pad array. - Andreas Sandberg On Nov. 1, 2016, 7:36 p.m., Tony Gutierrez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3690/ > ----------------------------------------------------------- > > (Updated Nov. 1, 2016, 7:36 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11888:ed39f10e4ff3 > --------------------------- > x86, ext: fix buf overflow in fp80 ops; pad fp80_t in fputils > > the compiler seems to align the fp80_t data struct, so here we add > explicit padding to avoid confusion. > > storeFloat80() will try to write all 16B of the fp80_t to the bits[] array > of the calling instruction. this happens because storeFloat80() points its > local fp80_t* to the memory the caller allocated for bits[], which is only > 10B, thus we get an overflow that is flagged by clang's asan. here we > get the fp80 value first, the memcpy() the bits[] of fp80_t to the mem > allocated by the caller. > > > Diffs > ----- > > ext/fputils/fpbits.h c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > ext/fputils/include/fputils/fptypes.h > c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/arch/x86/isa/microops/fpop.isa c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/arch/x86/utility.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > > Diff: http://reviews.gem5.org/r/3690/diff/ > > > Testing > ------- > > > Thanks, > > Tony Gutierrez > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
