Thanks for the patch, Nicolas. It looks like it doesn't apply cleanly, though. Could you repost by using "hg postreview -o"? Also, be sure to use the gem5 development repo as the base for your changeset, not gem5-stable.

Based on recent experiences, please make sure it compiles on OSX

src/sim/syscall_emul.hh (Diff revision 2)

whitespace here and a few other places
is your editor setup correctly?

Show all issues

1. Nicolas Derumigny Aug. 8, 2016, 2:21 a.m. (Aug. 8, 2016, 2:21 a.m.)

   My bad, I left some whitespace du to auto spacing, It's corrected now. Sorry, I do not have some OSX computer to test with right now, how can I test ? I can put preprocessor directives to let it run only on linux.

2. Jason Lowe-Power Aug. 8, 2016, 6:49 a.m. (Aug. 8, 2016, 6:49 a.m.)

   Andreas, how do you suggest those of us without access to OSX test the compile? I agree this is important since many of our developers seem to use Macs, but it is also a little much to ask for all of us to go out and buy a Mac to make sure gem5 compiles. From what I can tell online, it's of questionable legality to run an OSX image in vmware or other virutalization systems. Any ideas?

3. Andreas Hansson Aug. 8, 2016, 9:14 a.m. (Aug. 8, 2016, 9:14 a.m.)

   I realise it's not feasible to ask everyone to compile on OSX. Apologies if I came across that way.
   That said, when submitting patches that are closely tied to the underlying OS (syscall emul etc) it is definitely a big plus if they can be tested on a variety of platforms. It's also a request to other gem5 users to help out and test on their machines.

LGTM. Two questions about testing, though.

1) Did you test the 32-bit versions?

2) Can you add tests for these? It seems like you have some small bits of code. If they only take a few seconds to run in gem5, you should make a new regression test.
Thanks!

Hi Nicolas, thanks for the contribution. Couple of things before you commit this.

src/sim/syscall_emul.hh (Diff revision 4)

alignment should be under the 'S' in "SyscallDesc"

Show all issues

src/sim/syscall_emul.hh (Diff revision 4)

alignment

Show all issues

src/sim/syscall_emul.cc (Diff revision 4)

Please rename fd to sim_fd. It gets confusing over time if the distinction isn't explicit especially in more complicated functions. I've seen at least one bug that was caused by this type of naming.

Show all issues

src/sim/syscall_emul.cc (Diff revision 4)

This should not be an assert here. I think the correct thing to do here is instead "if(fd < 0) {return -EBADF;}".
The point is that the application is free to pass in bad file descriptors to the kernel. The kernel's job is to reject the file descriptor and return an errno to the application. It's not the kernel's job to kill the process for passing in a bad file descriptor.

Show all issues

src/sim/syscall_emul.cc (Diff revision 4)

Local variables are supposed to be lower case and seperated by underscores (instead of camel case).
http://www.m5sim.org/Coding_Style

Show all issues

src/sim/syscall_emul.cc (Diff revision 4)

variable name

Show all issues

src/sim/syscall_emul.cc (Diff revision 4)

save errno to a local variable after the system call

Show all issues

src/sim/syscall_emul.cc (Diff revision 4)

space between if keyword and paren

Show all issues

src/sim/syscall_emul.cc (Diff revision 4)

add a conditional return here if the sim_fd is bad

Show all issues

src/sim/syscall_emul.cc (Diff revision 4)

rename fd to sim_fd

Show all issues

src/sim/syscall_emul.cc (Diff revision 4)

name

Show all issues

src/sim/syscall_emul.cc (Diff revision 4)

name

Show all issues

src/sim/syscall_emul.cc (Diff revision 4)

You should save the errno value into a local variable immediately after a real system call if you have intervening code between the system call and the code.
Otherwise, the errno value might get clobbered before you return it.

Show all issues

src/sim/syscall_emul.cc (Diff revision 4)

space

Show all issues

Ship It!

src/arch/x86/linux/process.cc (Diff revision 5)

What is the reasoning behind this change?
The system calls are defined by what the Linux kernel has set for them; we can't arbitrarily decide to alter the call to a different variant.

Show all issues

1. Nicolas Derumigny Aug. 27, 2016, 1:32 a.m. (Aug. 27, 2016, 1:32 a.m.)

   It was a typing error, I wanted to set "getdents64" and getdents64Func.

src/sim/syscall_emul.hh (Diff revision 5)

remove whitespace on lines 305, 307, 311, and 313

Show all issues

src/sim/syscall_emul.cc (Diff revision 5)

space around '='

Show all issues

src/sim/syscall_emul.cc (Diff revision 5)

space around '='

Show all issues