[gem5-dev] Review Request 3780: riscv: Fix crash when syscall argument reg index is too high

2017-01-12 Thread Alec Roelke 

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

Review request for Default.


Repository: gem5


Description
---

Changeset 11795:44b4eec15cd7
---
riscv: Fix crash when syscall argument reg index is too high

By default, doSyscall gets the values of six registers to be used for
system call arguments.  RISC-V, by convention, only has four.  Because
RISC-V's implementation of these indices is as arrays of integers rather
than as base indices plus offsets, trying to get the fifth argument
register's value will cause a crash.  This patch fixes that by providing
the fourth register's value for any index higher than 3.


Diffs
-

  src/arch/riscv/process.cc 97eebddaae84 

Diff: http://reviews.gem5.org/r/3780/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 3780: riscv: Fix crash when syscall argument reg index is too high

2017-01-18 Thread Jason Lowe-Power 

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



src/arch/riscv/process.cc (line 222)


Two comments:
1. Shouldn't this at least have a warning? If the syscall arg register > 3 
there is definitely a bug, right?
2. Can you return "0" instead of the last register? I think that's a better 
failover case.


- Jason Lowe-Power


On Jan. 12, 2017, 9 p.m., Alec Roelke wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3780/
> ---
> 
> (Updated Jan. 12, 2017, 9 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11795:44b4eec15cd7
> ---
> riscv: Fix crash when syscall argument reg index is too high
> 
> By default, doSyscall gets the values of six registers to be used for
> system call arguments.  RISC-V, by convention, only has four.  Because
> RISC-V's implementation of these indices is as arrays of integers rather
> than as base indices plus offsets, trying to get the fifth argument
> register's value will cause a crash.  This patch fixes that by providing
> the fourth register's value for any index higher than 3.
> 
> 
> Diffs
> -
> 
>   src/arch/riscv/process.cc 97eebddaae84 
> 
> Diff: http://reviews.gem5.org/r/3780/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 3780: riscv: Fix crash when syscall argument reg index is too high

2017-01-18 Thread Alec Roelke 


> On Jan. 18, 2017, 4:06 p.m., Jason Lowe-Power wrote:
> > src/arch/riscv/process.cc, line 222
> > 
> >
> > Two comments:
> > 1. Shouldn't this at least have a warning? If the syscall arg register 
> > > 3 there is definitely a bug, right?
> > 2. Can you return "0" instead of the last register? I think that's a 
> > better failover case.

Returning 0 would make more sense, so I'll do that.  I can also add a warning, 
but that causes a minimum of two warnings to print every system call since the 
doSyscall() function gathers the values of the system call registers from 
indices 0 to 5 every time a system call is made.  This is only used for a debug 
statement, though; is there a reason it does this even when debugging is off?


- Alec


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


On Jan. 12, 2017, 9 p.m., Alec Roelke wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3780/
> ---
> 
> (Updated Jan. 12, 2017, 9 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11795:44b4eec15cd7
> ---
> riscv: Fix crash when syscall argument reg index is too high
> 
> By default, doSyscall gets the values of six registers to be used for
> system call arguments.  RISC-V, by convention, only has four.  Because
> RISC-V's implementation of these indices is as arrays of integers rather
> than as base indices plus offsets, trying to get the fifth argument
> register's value will cause a crash.  This patch fixes that by providing
> the fourth register's value for any index higher than 3.
> 
> 
> Diffs
> -
> 
>   src/arch/riscv/process.cc 97eebddaae84 
> 
> Diff: http://reviews.gem5.org/r/3780/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 3780: riscv: Fix crash when syscall argument reg index is too high

2017-01-18 Thread Jason Lowe-Power 


> On Jan. 18, 2017, 4:06 p.m., Jason Lowe-Power wrote:
> > src/arch/riscv/process.cc, line 222
> > 
> >
> > Two comments:
> > 1. Shouldn't this at least have a warning? If the syscall arg register 
> > > 3 there is definitely a bug, right?
> > 2. Can you return "0" instead of the last register? I think that's a 
> > better failover case.
> 
> Alec Roelke wrote:
> Returning 0 would make more sense, so I'll do that.  I can also add a 
> warning, but that causes a minimum of two warnings to print every system call 
> since the doSyscall() function gathers the values of the system call 
> registers from indices 0 to 5 every time a system call is made.  This is only 
> used for a debug statement, though; is there a reason it does this even when 
> debugging is off?

Ah. In this case, don't worry about adding a "warn". We'll just have to assume 
that anyone who implements a syscall in RISC-V will know not to use the values 
in registers > 3.

It would be good to add a comment when you update to return 0.


- Jason


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


On Jan. 12, 2017, 9 p.m., Alec Roelke wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3780/
> ---
> 
> (Updated Jan. 12, 2017, 9 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11795:44b4eec15cd7
> ---
> riscv: Fix crash when syscall argument reg index is too high
> 
> By default, doSyscall gets the values of six registers to be used for
> system call arguments.  RISC-V, by convention, only has four.  Because
> RISC-V's implementation of these indices is as arrays of integers rather
> than as base indices plus offsets, trying to get the fifth argument
> register's value will cause a crash.  This patch fixes that by providing
> the fourth register's value for any index higher than 3.
> 
> 
> Diffs
> -
> 
>   src/arch/riscv/process.cc 97eebddaae84 
> 
> Diff: http://reviews.gem5.org/r/3780/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 3780: riscv: Fix crash when syscall argument reg index is too high

2017-01-18 Thread Alec Roelke 

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

(Updated Jan. 18, 2017, 5:39 p.m.)


Review request for Default.


Repository: gem5


Description (updated)
---

Changeset 11795:3ac8305dbf60
---
riscv: Fix crash when syscall argument reg index is too high

By default, doSyscall gets the values of six registers to be used for
system call arguments.  RISC-V, by convention, only has four.  Because
RISC-V's implementation of these indices is as arrays of integers rather
than as base indices plus offsets, trying to get the fifth argument
register's value will cause a crash.  This patch fixes that by returning 0
for any index higher than 3.


Diffs (updated)
-

  src/arch/riscv/process.cc 97eebddaae84 

Diff: http://reviews.gem5.org/r/3780/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 3780: riscv: Fix crash when syscall argument reg index is too high

2017-01-18 Thread Jason Lowe-Power 

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

Ship it!


Ship It!

- Jason Lowe-Power


On Jan. 18, 2017, 5:39 p.m., Alec Roelke wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3780/
> ---
> 
> (Updated Jan. 18, 2017, 5:39 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11795:3ac8305dbf60
> ---
> riscv: Fix crash when syscall argument reg index is too high
> 
> By default, doSyscall gets the values of six registers to be used for
> system call arguments.  RISC-V, by convention, only has four.  Because
> RISC-V's implementation of these indices is as arrays of integers rather
> than as base indices plus offsets, trying to get the fifth argument
> register's value will cause a crash.  This patch fixes that by returning 0
> for any index higher than 3.
> 
> 
> Diffs
> -
> 
>   src/arch/riscv/process.cc 97eebddaae84 
> 
> Diff: http://reviews.gem5.org/r/3780/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 3780: riscv: Fix crash when syscall argument reg index is too high

2017-01-19 Thread Brandon Potter 

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

Ship it!


Ship It!

- Brandon Potter


On Jan. 18, 2017, 5:39 p.m., Alec Roelke wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3780/
> ---
> 
> (Updated Jan. 18, 2017, 5:39 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11795:3ac8305dbf60
> ---
> riscv: Fix crash when syscall argument reg index is too high
> 
> By default, doSyscall gets the values of six registers to be used for
> system call arguments.  RISC-V, by convention, only has four.  Because
> RISC-V's implementation of these indices is as arrays of integers rather
> than as base indices plus offsets, trying to get the fifth argument
> register's value will cause a crash.  This patch fixes that by returning 0
> for any index higher than 3.
> 
> 
> Diffs
> -
> 
>   src/arch/riscv/process.cc 97eebddaae84 
> 
> Diff: http://reviews.gem5.org/r/3780/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alec Roelke
> 
>

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