Page MenuHomePhabricator

[scudo][standalone] Add flags & related parsers
ClosedPublic

Authored by cryptoad on Mar 20 2019, 8:24 AM.

Details

Summary

As with other Sanitizers, and the current version of Scudo, we can
provide flags in differents way: at compile time, through a weak
function, through an environment variable.

This change adds support for the configuration flags, and the string
parsers. Those are fairly similar to the sanitizer_common way of doing
things.

Diff Detail

Event Timeline

cryptoad created this revision.Mar 20 2019, 8:24 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 20 2019, 8:24 AM
Herald added subscribers: Restricted Project, jdoerfert, delcypher, mgorny. · View Herald Transcript
cryptoad updated this revision to Diff 191507.Mar 20 2019, 8:56 AM

Forgot a couple of headers in the makefile.

morehouse added inline comments.Mar 20 2019, 11:10 AM
lib/scudo/standalone/flags_parser.cc
160

Could we just have Flags be a static array?

lib/scudo/standalone/flags_parser.h
27

Do we need inheritance for Scudo?

37

Why do we need operator delete?

cryptoad planned changes to this revision.Mar 20 2019, 1:38 PM
cryptoad added inline comments.
lib/scudo/standalone/flags_parser.h
27

Actually that's a good point I might be able to do without. Let me revisit that.

cryptoad updated this revision to Diff 193884.Apr 5 2019, 8:22 AM

This new version of the flags parsing departs from the one currently
implemented in sanitizer_common. We avoid dynamic memory allocations
by doing all the parsing in place, and only support boolean and integer
flags (strings would require to be duplicated to have them null
terminated).

This addresses some of the comments from Matt & Mitch:

  • no more map call for the flags, they are static
  • no more placement new and sketchy delete declaration (even though

those were never destroyed, compiling on Fuchsia required a delete
definition that was left empty for the classes)

  • no more virtual class and inheritence

This comes with some caveats, like the fact that since we are not
null terminating the strings, displaying a flag (eg: Printf of
unknown flags or the like) will output the whole string passed.
I felt it wasn't a big deal.

morehouse added inline comments.Apr 5 2019, 2:58 PM
lib/scudo/standalone/flags_parser.cc
68

Let's add tests that exercise this path, or just not support quotes.

80

Unreachable condition?

128

Might be cleaner to test the inverse with a continue.

lib/scudo/standalone/flags_parser.h
22

The FT_ prefix seems redundant since we'll always have to prefix with FlagType:: anyway.

I suppose it does make things simpler in registerFlags though...

lib/scudo/standalone/tests/flags_test.cc
78

Wild semicolon

vitalybuka added inline comments.Apr 5 2019, 5:41 PM
lib/scudo/standalone/flags.cc
17

Why not

Flags *getFlags() { 
   static Flags F;
   return &F; 
}
lib/scudo/standalone/flags.inc
14

Maybe more convenient is to add particular flags in patches which use them?

lib/scudo/standalone/flags_parser.cc
49

isSpace is no exactly checking for space

Could we make code simple if we add check for null here?

lib/scudo/standalone/flags_parser.h
44

It would be more readable if field are initialized here

cryptoad marked 9 inline comments as done.Apr 8 2019, 9:08 AM
cryptoad added inline comments.
lib/scudo/standalone/flags.inc
14

My idea was more the contrary: once a file is checked in, avoid further changes unless something comes up later.
I am not opposed to doing it your way, let me know.

lib/scudo/standalone/flags_parser.cc
49

I renamed this to isSeparator, which sounds more appropriate.
As for checking for null, the parseFlags loop requires to make a distinction between the whitespace/separators & the null character which will be termination.
So I think it should stay that way, unless I am not seeing something.

lib/scudo/standalone/flags_parser.h
22

I felt it worked out well with a concatenation macro with the type. It sorts of departs from the casing style, but it allowed to use the type (bool, int) as is.

44

I am unclear as to what fields you are referring to, the ones initialized in the constructor?

cryptoad updated this revision to Diff 194161.Apr 8 2019, 9:15 AM

Addressing some review comments:

  • adding tests for quoted flags
  • code changes as requested
vitalybuka added inline comments.Apr 8 2019, 10:25 AM
lib/scudo/standalone/flags.inc
14

Up to you :-)

lib/scudo/standalone/flags_parser.cc
49

Only skipWhitespace does not check NULL

So you can either:

void FlagParser::skipWhitespace() {
  while (Buffer[Pos] && isSeparator(Buffer[Pos]))
    ++Pos;
}

or just add new method

bool FlagParser::isSeparatorOrNull(char C) {
  return !C || IsSeparator(C);
}
// before
(Buffer[Pos] != 0 && Buffer[Pos] != '=' && !isSeparator(Buffer[Pos]))
// after
(Buffer[Pos] != '=' && !isSeparatorOrNull(Buffer[Pos]))

// before
(Buffer[Pos] != 0 && !isSeparator(Buffer[Pos]))
// after
(!isSeparatorOrNull(Buffer[Pos]))

// before
*ValueEnd == 0 || *ValueEnd == '"' || *ValueEnd == '\'' ||  isSeparator(*ValueEnd);
// after
*ValueEnd == '"' || *ValueEnd == '\'' ||  isSeparatorOrNull(*ValueEnd);
49

Not sure why this is in header file.
I'd hide as much as possible into
namespace {} of cc file

BTW. cc is google3, llvm is cpp

lib/scudo/standalone/flags_parser.h
44

right

const char *Buffer = null;
uptr Pos = 0;
morehouse accepted this revision.Apr 8 2019, 10:28 AM

LGTM after addressing Vitaly's comments.

This revision is now accepted and ready to land.Apr 8 2019, 10:28 AM
cryptoad marked 11 inline comments as done.Apr 8 2019, 12:36 PM
cryptoad added inline comments.
lib/scudo/standalone/flags_parser.cc
49

Right, good point, remove it from the header!
As for the .cc vs .cpp, I am not sure why I started with that, and that's my bad. I will rename them all in a later CL.

cryptoad updated this revision to Diff 194194.Apr 8 2019, 12:38 PM
cryptoad marked an inline comment as done.

Following up on some good catches by Vitaly:

  • changing the isSeparator code around to not be in the class
  • adding a related function that also checks for null
  • doing some inline initialization of the FlagParser members
vitalybuka accepted this revision.Apr 8 2019, 1:07 PM
This revision was automatically updated to reflect the committed changes.