Re: [gem5-dev] Review Request 3693: riscv: [Patch 7/5] Corrected LRSC semantics

[Alec Roelke]

> On Nov. 10, 2016, 4:37 p.m., Tony Gutierrez wrote:
> > src/arch/riscv/locked_mem.hh, line 96
> > 
> >
> > Should you be using cacheBlockMask here, as opposed to 0xF?

That would make more sense to me, also, but cacheBlockMask isn't a parameter 
for handleLockedRead.  In MIPS, which this is based off of, ~0xF is used here 
as well (and in handleLockedWrite even though handleLockedWrite *does* have a 
cacheBlockMask parameter).


- Alec


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3693/#review9030
---


On Nov. 2, 2016, 7:34 p.m., Alec Roelke wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3693/
> ---
> 
> (Updated Nov. 2, 2016, 7:34 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11694:1c3068cb5c86
> ---
> riscv: [Patch 7/5] Corrected LRSC semantics
> 
> RISC-V makes use of load-reserved and store-conditional instructions to
> enable creation of lock-free concurrent data manipulation as well as
> ACQUIRE and RELEASE semantics for memory ordering of LR, SC, and AMO
> instructions (the latter of which do not follow LR/SC semantics). This
> patch is a correction to patch 4, which added these instructions to the
> implementation of RISC-V. It modifies locked_mem.hh and the
> implementations of lr.w, sc.w, lr.d, and sc.d to apply the proper gem5
> flags and return the proper values.
> 
> An important difference between gem5's LLSC semantics and RISC-V's LR/SC
> ones, beyond the name, is that gem5 uses 0 to indicate failure and 1 to
> indicate success, while RISC-V is the opposite. Strictly speaking, RISC-V
> uses 0 to indicate success and nonzero to indicate failure where the
> value would indicate the error, but currently only 1 is reserved as a
> failure code by the ISA reference.
> 
> This is the seventh patch in the series which originally consisted of five
> patches that added the RISC-V ISA to gem5. The original five patches added
> all of the instructions and added support for more detailed CPU models and
> the sixth patch corrected the implementations of Linux constants and
> structs. There will be an eighth patch that adds some regression tests
> for the instructions.
> 
> Signed-off by: Alec Roelke
> 
> 
> Diffs
> -
> 
>   src/arch/riscv/isa/decoder.isa PRE-CREATION 
>   src/arch/riscv/isa/formats/mem.isa PRE-CREATION 
>   src/arch/riscv/locked_mem.hh PRE-CREATION 
> 
> Diff: http://reviews.gem5.org/r/3693/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alec Roelke
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3528: misc: add a TLM to Gem5 Master Port

[Andreas Hansson]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3528/#review9081
---


Thanks for this. Some minor requests and style issues, then it's good to go.


util/tlm/sc_master_port.cc (line 74)


It would be good to add a comment explaining where the request is deleted.



util/tlm/sc_master_port.cc (line 232)


this is not a safe assumption

use SimClock::Int::ps for the "conversion"



util/tlm/sc_master_port.cc (line 289)


do things work even if this is not TLM_UPDATED?


- Andreas Hansson


On Nov. 7, 2016, 2:56 p.m., Christian Menard wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3528/
> ---
> 
> (Updated Nov. 7, 2016, 2:56 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11707:cb37987f081e
> ---
> misc: add a TLM to Gem5 Master Port
> 
> The current TLM code only provides a Slave Port that allows the gem5 world to
> send requests to the the TLM world. This patch adds a Master Port that allows
> the TLM world to send requests to the gem5 world. Furthermore, the patch
> provides a simple example application based on a TLM traffic generator.
> 
> 
> Diffs
> -
> 
>   util/tlm/sc_master_port.cc PRE-CREATION 
>   util/tlm/sim_control.cc PRE-CREATION 
>   util/tlm/examples/master_port/traffic_generator.hh PRE-CREATION 
>   util/tlm/examples/master_port/traffic_generator.cc PRE-CREATION 
>   util/tlm/sc_master_port.hh PRE-CREATION 
>   util/tlm/README b3d5f0e9e258 
>   util/tlm/examples/master_port/SConstruct PRE-CREATION 
>   util/tlm/examples/master_port/main.cc PRE-CREATION 
>   util/tlm/examples/master_port/tlm.py PRE-CREATION 
> 
> Diff: http://reviews.gem5.org/r/3528/diff/
> 
> 
> Testing
> ---
> 
> A simple example application consisting of a TLM traffic generator and a gem5 
> memory is part of the patch.
> 
> 
> Thanks,
> 
> Christian Menard
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

[gem5-dev] Review Request 3705: dev: Fix buffer length when unserializing an eth pkt

[Michael LeBeane]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3705/
---

Review request for Default.


Repository: gem5


Description
---

Changeset 11705:008f04dcc085
---
dev: Fix buffer length when unserializing an eth pkt
Changeset 11701 only serialized the useful portion of of an ethernet packets'
payload.  However, the device models expect each ethernet packet to contain
a 16KB buffer, even if there is no data in it.  This patch adds a 'bufLength'
field to EthPacketData so the original size of the packet buffer can always be
unserialized.

Reported-by: Gabor Dozsa 


Diffs
-

  src/dev/net/etherpkt.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
  src/dev/net/etherpkt.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 

Diff: http://reviews.gem5.org/r/3705/diff/


Testing
---


Thanks,

Michael LeBeane

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3701: syscall_emul: [PATCH 20/22] add the tgkill system call

[Michael LeBeane]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3701/#review9080
---

Ship it!


Ship It!

- Michael LeBeane


On Nov. 14, 2016, 8:56 p.m., Brandon Potter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3701/
> ---
> 
> (Updated Nov. 14, 2016, 8:56 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11727:33fae597297e
> ---
> syscall_emul: [PATCH 20/22] add the tgkill system call
> 
> 
> Diffs
> -
> 
>   src/sim/syscall_emul.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/arch/x86/linux/process.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
> 
> Diff: http://reviews.gem5.org/r/3701/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Brandon Potter
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3702: syscall_emul: [PATCH 21/22] rewrite code related to system call exits

[Michael LeBeane]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3702/#review9079
---

Ship it!


Ship It!

- Michael LeBeane


On Nov. 14, 2016, 9:30 p.m., Brandon Potter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3702/
> ---
> 
> (Updated Nov. 14, 2016, 9:30 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11728:9ca77be78b02
> ---
> syscall_emul: [PATCH 21/22] rewrite code related to system call exits
> 
> The changeset refactors exit, exit_group, and futex related exit
> functionality.
> 
> 
> Diffs
> -
> 
>   src/sim/syscall_emul.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/se_signal.hh PRE-CREATION 
>   src/sim/futex_map.hh PRE-CREATION 
>   src/sim/process.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/syscall_emul.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/system.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/se_signal.cc PRE-CREATION 
> 
> Diff: http://reviews.gem5.org/r/3702/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Brandon Potter
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3704: style: change NULL to nullptr in syscall files

[Michael LeBeane]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3704/#review9078
---

Ship it!


Ship It!

- Michael LeBeane


On Nov. 14, 2016, 9:02 p.m., Brandon Potter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3704/
> ---
> 
> (Updated Nov. 14, 2016, 9:02 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11730:3b22b3ca4675
> ---
> style: change NULL to nullptr in syscall files
> 
> 
> Diffs
> -
> 
>   src/sim/syscall_emul.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/syscall_emul.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
> 
> Diff: http://reviews.gem5.org/r/3704/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Brandon Potter
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3696: syscall_emul: [PATCH 15/22] add clone/execve for threading and multiprocess simulations

[Michael LeBeane]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3696/#review9076
---


In general, I think this patch could be improved by making better use of copy 
constructors and overloaded assignment operators.  I can review again in more 
detail once that has been addressed.


src/arch/x86/process.cc (line 111)


Implementing assigment operator would be nice.



src/arch/x86/process.cc (line 1100)


Same here.



src/arch/x86/process.cc (line 1138)


Same here.



src/cpu/thread_state.hh (line 113)


What is this proxy stuff?  Some comments would help.



src/mem/page_table.cc (line 107)


Name is a bit weird IMO.  getMappings make it seem like this is a getter, 
which it isn't.  Maybe something like appendMappings?  Also should be const.



src/sim/Process.py (line 45)


Why change this?



src/sim/process.hh (line 190)


Seems like this should be an assert() instead of a silent return.



src/sim/process.hh (line 264)


I prefer shared_pointer here.  Also could use some comments explaining what 
these fields are and that they can be shared accross different processes.



src/sim/process.cc (line 161)


Unnecessary.



src/sim/process.cc (line 166)


Looking at how you use this, maybe the right thing to do is to just make 
getMappings an actual getter:

MapVec mappings = pTable->getMappings();



src/sim/process.cc (line 212)


np->argv.insert(np->argv.end(), argv.begin(), argv.end());

Seems cleaner.  Same for envp.



src/sim/process.cc (line 259)


Should we panic here?



src/sim/process.cc (line 338)


What about the other variables you introduce, such as exitGroup, sigchld, 
maxStackSize, etc?  Does checkpointing even work?  If not, there should be a 
warning stating what is not supported.



src/sim/syscall_desc.hh (line 110)


const



src/sim/syscall_emul.hh (line 1183)


Seems like there should be a cleaner way to do this with a copy constructor.



src/sim/syscall_emul.hh (line 1689)


In general, there is too much duplication between this function and 
cloneFunc.  For example, some the ProcessParams stuff seems to be copied.  This 
should be factored out a bit better.


- Michael LeBeane


On Nov. 14, 2016, 8:37 p.m., Brandon Potter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3696/
> ---
> 
> (Updated Nov. 14, 2016, 8:37 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11722:6fea3eedd891
> ---
> syscall_emul: [PATCH 15/22] add clone/execve for threading and multiprocess 
> simulations
> 
> Modifies the clone system call and adds execve system call. Requires allowing
> processes to steal thread contexts from other processes in the same system
> object and the ability to detach pieces of process state (such as MemState)
> to allow dynamic sharing.
> 
> 
> Diffs
> -
> 
>   src/sim/syscall_emul.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/syscall_desc.hh PRE-CREATION 
>   src/sim/syscall_emul.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/process.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/process.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/mem/page_table.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/mem/se_translating_port_proxy.hh 
> c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/Process.py c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/cpu/thread_state.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/mem/page_table.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/cpu/thread_context.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/cpu/o3/thread_context.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/cpu/simple_thread.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/arch/x86/types.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/cpu/checker/thread_context.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   

Re: [gem5-dev] Review Request 3690: x86, ext: fix buf overflow in fp80 ops; pad fp80_t in fputils

[Tony Gutierrez]

> On Nov. 15, 2016, 9:27 a.m., Andreas Sandberg wrote:
> > src/arch/x86/isa/microops/fpop.isa, line 354
> > 
> >
> > Are these changes really needed?

Fixed. I will push this once the changes to fputils have been pulled into gem5.


- Tony


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3690/#review9075
---


On Nov. 15, 2016, 11:52 a.m., Tony Gutierrez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3690/
> ---
> 
> (Updated Nov. 15, 2016, 11:52 a.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11894:be1b22d0c36a
> ---
> 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.
> 
> some of the x86 FP ops also use char to represent 8b types, while the fp80
> struct uses uint8_t, so here we make the x86 ops use uint8_t as well.
> 
> 
> Diffs
> -
> 
>   src/arch/x86/utility.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
> 
> Diff: http://reviews.gem5.org/r/3690/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tony Gutierrez
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3690: x86, ext: fix buf overflow in fp80 ops; pad fp80_t in fputils

[Tony Gutierrez]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3690/
---

(Updated Nov. 15, 2016, 11:52 a.m.)


Review request for Default.


Repository: gem5


Description (updated)
---

Changeset 11894:be1b22d0c36a
---
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.

some of the x86 FP ops also use char to represent 8b types, while the fp80
struct uses uint8_t, so here we make the x86 ops use uint8_t as well.


Diffs (updated)
-

  src/arch/x86/utility.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 

Diff: http://reviews.gem5.org/r/3690/diff/


Testing
---


Thanks,

Tony Gutierrez

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3690: x86, ext: fix buf overflow in fp80 ops; pad fp80_t in fputils

[Andreas Sandberg]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3690/#review9075
---

Ship it!



src/arch/x86/isa/microops/fpop.isa (line 354)


Are these changes really needed?


- Andreas Sandberg


On Nov. 15, 2016, 5:18 p.m., Tony Gutierrez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3690/
> ---
> 
> (Updated Nov. 15, 2016, 5:18 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11894:c211fc37d7a1
> ---
> 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.
> 
> some of the x86 FP ops also use char to represent 8b types, while the fp80
> struct uses uint8_t, so here we make the x86 ops use uint8_t as well.
> 
> 
> Diffs
> -
> 
>   src/arch/x86/utility.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   ext/fputils/fpbits.h c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   ext/fputils/include/fputils/fptypes.h 
> c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/arch/x86/isa/microops/fpop.isa c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
> 
> Diff: http://reviews.gem5.org/r/3690/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tony Gutierrez
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3690: x86, ext: fix buf overflow in fp80 ops; pad fp80_t in fputils

[Andreas Sandberg]

> On Nov. 15, 2016, 3:16 p.m., Andreas Sandberg wrote:
> > 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 ?
> 
> Tony Gutierrez wrote:
> Sure. Are you asking me to not push this into gem5 though?

I would prefer if we could push it to the external library first and then pull 
in that revision into ext. This makes it easier to track the upstream revision 
in the change log.


- Andreas


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3690/#review9072
---


On Nov. 15, 2016, 5:18 p.m., Tony Gutierrez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3690/
> ---
> 
> (Updated Nov. 15, 2016, 5:18 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11894:c211fc37d7a1
> ---
> 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.
> 
> some of the x86 FP ops also use char to represent 8b types, while the fp80
> struct uses uint8_t, so here we make the x86 ops use uint8_t as well.
> 
> 
> Diffs
> -
> 
>   src/arch/x86/utility.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   ext/fputils/fpbits.h c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   ext/fputils/include/fputils/fptypes.h 
> c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/arch/x86/isa/microops/fpop.isa c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
> 
> Diff: http://reviews.gem5.org/r/3690/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tony Gutierrez
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3690: x86, ext: fix buf overflow in fp80 ops; pad fp80_t in fputils

[Tony Gutierrez]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3690/
---

(Updated Nov. 15, 2016, 9:18 a.m.)


Review request for Default.


Repository: gem5


Description (updated)
---

Changeset 11894:c211fc37d7a1
---
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.

some of the x86 FP ops also use char to represent 8b types, while the fp80
struct uses uint8_t, so here we make the x86 ops use uint8_t as well.


Diffs (updated)
-

  src/arch/x86/utility.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
  ext/fputils/fpbits.h c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
  ext/fputils/include/fputils/fptypes.h 
c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
  src/arch/x86/isa/microops/fpop.isa c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 

Diff: http://reviews.gem5.org/r/3690/diff/


Testing
---


Thanks,

Tony Gutierrez

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3690: x86, ext: fix buf overflow in fp80 ops; pad fp80_t in fputils

[Tony Gutierrez]

> On Nov. 15, 2016, 7:16 a.m., Andreas Sandberg wrote:
> > 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 ?

Sure. Are you asking me to not push this into gem5 though?


- Tony


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3690/#review9072
---


On Nov. 1, 2016, 12: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, 12: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
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3690: x86, ext: fix buf overflow in fp80 ops; pad fp80_t in fputils

[Andreas Sandberg]

---
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)


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
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

[gem5-dev] Cron <m5test@zizzer> /z/m5/regression/do-regression quick

[Cron Daemon]

* build/HSAIL_X86/tests/opt/quick/se/04.gpu/x86/linux/gpu-ruby-GPU_RfO: 
CHANGED!
* build/ALPHA/tests/opt/quick/se/00.hello/alpha/linux/simple-timing-ruby: 
passed.
* build/ALPHA/tests/opt/quick/se/00.hello/alpha/tru64/minor-timing: passed.
* build/ALPHA/tests/opt/quick/se/00.hello/alpha/linux/minor-timing: 
passed.* build/ALPHA/tests/opt/quick/se/00.hello/alpha/linux/o3-timing: 
passed.
* build/ALPHA/tests/opt/quick/se/00.hello/alpha/linux/simple-atomic: passed.
* build/ALPHA/tests/opt/quick/se/00.hello/alpha/linux/simple-timing: passed.
* build/ALPHA/tests/opt/quick/se/00.hello/alpha/tru64/o3-timing: passed.
* build/ALPHA/tests/opt/quick/se/00.hello/alpha/tru64/simple-atomic: passed.
* build/ALPHA/tests/opt/quick/se/00.hello/alpha/tru64/simple-timing: passed.
* build/ALPHA/tests/opt/quick/se/00.hello/alpha/tru64/simple-timing-ruby: 
passed.
* build/ALPHA/tests/opt/quick/se/01.hello-2T-smt/alpha/linux/o3-timing-mt: 
passed.
* build/ALPHA/tests/opt/quick/se/50.memtest/alpha/linux/memtest-ruby: 
passed.
 * 
build/ALPHA/tests/opt/quick/se/03.learning-gem5/alpha/linux/learning-gem5-p1-simple:
 passed.
* 
build/ALPHA/tests/opt/quick/se/03.learning-gem5/alpha/linux/learning-gem5-p1-two-level:
 passed.
* build/ALPHA/tests/opt/quick/se/30.eon/alpha/tru64/simple-atomic: passed.
* build/ALPHA/tests/opt/quick/se/50.vortex/alpha/tru64/simple-atomic: 
passed.
* build/ALPHA/tests/opt/quick/se/50.vortex/alpha/tru64/simple-timing: 
passed.
* build/ALPHA/tests/opt/quick/se/60.rubytest/alpha/linux/rubytest-ruby: 
passed.
* 
build/ALPHA/tests/opt/quick/fs/10.linux-boot/alpha/linux/tsunami-simple-atomic: 
passed.
* 
build/ALPHA/tests/opt/quick/fs/10.linux-boot/alpha/linux/tsunami-simple-atomic-dual:
 passed.
* build/ALPHA/tests/opt/quick/se/70.twolf/alpha/tru64/simple-atomic: passed.
* 
build/ALPHA/tests/opt/quick/fs/10.linux-boot/alpha/linux/tsunami-simple-timing: 
passed.
* 
build/ALPHA/tests/opt/quick/fs/10.linux-boot/alpha/linux/tsunami-simple-timing-dual:
 passed.
* build/ALPHA/tests/opt/quick/se/70.twolf/alpha/tru64/simple-timing: passed.
* 
build/ALPHA/tests/opt/quick/fs/80.netperf-stream/alpha/linux/twosys-tsunami-simple-atomic:
 passed.
* 
build/ALPHA_MOESI_hammer/tests/opt/quick/se/00.hello/alpha/linux/simple-timing-ruby-MOESI_hammer:
 passed.
* 
build/ALPHA_MOESI_hammer/tests/opt/quick/se/00.hello/alpha/tru64/simple-timing-ruby-MOESI_hammer:
 passed.
* 
build/ALPHA_MOESI_hammer/tests/opt/quick/se/50.memtest/alpha/linux/memtest-ruby-MOESI_hammer:
 passed.
* 
build/ALPHA_MOESI_hammer/tests/opt/quick/se/60.rubytest/alpha/linux/rubytest-ruby-MOESI_hammer:
 passed.
* 
build/ALPHA_MESI_Two_Level/tests/opt/quick/se/00.hello/alpha/tru64/simple-timing-ruby-MESI_Two_Level:
 passed.
* 
build/ALPHA_MESI_Two_Level/tests/opt/quick/se/50.memtest/alpha/linux/memtest-ruby-MESI_Two_Level:
 passed.
* 
build/ALPHA_MESI_Two_Level/tests/opt/quick/se/00.hello/alpha/linux/simple-timing-ruby-MESI_Two_Level:
 passed.
* 
build/ALPHA_MESI_Two_Level/tests/opt/quick/se/60.rubytest/alpha/linux/rubytest-ruby-MESI_Two_Level:
 passed.
* 
build/ALPHA_MOESI_CMP_directory/tests/opt/quick/se/50.memtest/alpha/linux/memtest-ruby-MOESI_CMP_directory:
 passed.
* 
build/ALPHA_MOESI_CMP_directory/tests/opt/quick/se/60.rubytest/alpha/linux/rubytest-ruby-MOESI_CMP_directory:
 passed.
* 
build/ALPHA_MOESI_CMP_directory/tests/opt/quick/se/00.hello/alpha/linux/simple-timing-ruby-MOESI_CMP_directory:
 passed.
* 
build/ALPHA_MOESI_CMP_directory/tests/opt/quick/se/00.hello/alpha/tru64/simple-timing-ruby-MOESI_CMP_directory:
 passed.
* 
build/ALPHA_MOESI_CMP_token/tests/opt/quick/se/50.memtest/alpha/linux/memtest-ruby-MOESI_CMP_token:
 passed.
* 
build/ALPHA_MOESI_CMP_token/tests/opt/quick/se/00.hello/alpha/tru64/simple-timing-ruby-MOESI_CMP_token:
 passed.
* 
build/ALPHA_MOESI_CMP_token/tests/opt/quick/se/60.rubytest/alpha/linux/rubytest-ruby-MOESI_CMP_token:
 passed.
 * 
build/ALPHA_MOESI_CMP_token/tests/opt/quick/se/00.hello/alpha/linux/simple-timing-ruby-MOESI_CMP_token:
 passed.
* build/MIPS/tests/opt/quick/se/00.hello/mips/linux/simple-timing-ruby: 
passed.
* build/MIPS/tests/opt/quick/se/00.hello/mips/linux/simple-atomic: passed.
* 
build/MIPS/tests/opt/quick/se/03.learning-gem5/mips/linux/learning-gem5-p1-two-level:
 passed.
* build/MIPS/tests/opt/quick/se/00.hello/mips/linux/simple-timing: passed.
* 
build/MIPS/tests/opt/quick/se/03.learning-gem5/mips/linux/learning-gem5-p1-simple:
 passed.
* build/MIPS/tests/opt/quick/se/00.hello/mips/linux/o3-timing: passed.
* build/NULL/tests/opt/quick/se/50.memtest/null/none/memtest-filter: passed.
* build/NULL/tests/opt/quick/se/50.memtest/null/none/memtest: passed.
* build/NULL/tests/opt/quick/se/51.memcheck/null/none/memcheck: passed.
* build/NULL/tests/opt/quick/se/70.tgen/null/none/tgen-dram-ctrl: passed.