A few minor issues, mostly related to pre-existing code. If you feel like addressing them while you're touching this code that would be great. Otherwise it looks good to me.

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

This is not the correct license. See the code in gpu-compute for the license we should be using.

Show all issues

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

(void) is not necessary in c++. also this is not gem5 convention.

Show all issues

1. Brandon Potter Oct. 17, 2016, 5:43 a.m. (Oct. 17, 2016, 5:43 a.m.)

   I tried to get the same change past Steve once and he shot me down with the same response. Sigh, I prefer my void being there (and it's completely legal), but I'll change it for make benefit of glorious gem5.
   Another common but unnecessary habit which I see quite a bit is using "unsigned int" instead of just "unsigned". We'll have to crack down on those folks now while we're at it. :)

2. Steve Reinhardt Oct. 19, 2016, 9:53 a.m. (Oct. 19, 2016, 9:53 a.m.)

   It's legal, but I believe it's only there to provide backwards compatibility with C.  And the only reason this construct even exists in C is to distinguish prototypes of functions with no parameters with old-school K&R function declarations that don't provide parameter information (and which are not part of C++, and really should not be in any C code written in the last 25 years).  So I think you should treat this feature as a historical anomaly and not as something you should be trying to exploit ;).

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

Can we use std::string here? Might as well change it now if you're touching this code anyhow.

Show all issues

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

Not a major issue, and I know you didn't add this either, but I think it'd be better to have a more explicit name for this: SysCallExecutor or something like that. Really anything more descriptive that FuncPtr would be ok.

Show all issues

Ship It!

Ship It!

Ship It!

Ship It!

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

my preference would be to keep this as a one-liner as in the original code. I don't think expanding it makes it more readable. If anything, it makes the code seem more significant than it really is.

Show all issues

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

if you're going to create methods, might as well put the semantics here too. i.e., no need for separate warned() and setWarned() methods, just do something like:
bool
checkWarned()
{
    bool was_warned = _warned;
    _warned = true;
    return was_warned;
}

and then the code in ignoreFunc() could be simplified a bit. I might even go further and, instead of checkWarned() like above, have a function like this:
bool
needWarning()
{
    bool suppress_warning = warnOnce() && !_warned;
    _warned = true;
    return !suppress_warning;
}

and then you could rewrite ignoreFunc() along the lines of:
if (desc->needWarning()) {
    warn("ignoring syscall %s(...)%s", desc->name(),
         desc->warnOnce() ? "\n      (further warnings will be suppressed)" : "");
}

Show all issues

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

again, this could be a one-liner.

Show all issues

Ship It!