|
From: Nicholas N. <n.n...@gm...> - 2009-02-24 04:24:08
Attachments:
diff
|
Hi,
atoll() is a terrible function -- you can't do any error checking with it.
Some of our option processing code uses it. This means that eg.
'--log-fd=9xxx' logs to fd 9, and '--log-fd=blahblahblah' logs to 0 (because
atoll() returns 0 if the string doesn't contain a number!)
It turns out that most of our option processing uses VG_(strtoll*) instead
of VG_(atoll). The reason that not all of it does is that the
option-processing macros are underpowered -- they currently work well if you
just want to assign the value to a variable, eg:
VG_BOOL_CLO(arg, "--heap", clo_heap)
else VG_BOOL_CLO(arg, "--stacks", clo_stacks)
else VG_NUM_CLO(arg, "--heap-admin", clo_heap_admin)
else VG_NUM_CLO(arg, "--depth", clo_depth)
(This works because the macros actually contain an if-statement, but
it looks odd.)
VG_NUM_CLO uses VG_(stroll10). But if you want to do any checking or
processing, you can't use those macros, leading to code like this:
else if (VG_CLO_STREQN(9, arg, "--log-fd=")) {
log_to = VgLogTo_Fd;
VG_(clo_log_name) = NULL;
tmp_log_fd = (Int)VG_(atoll)(&arg[9]);
}
So this patch:
- Improves the *_CLO_* macros so that they can be used in all circumstances.
They're now just expressions (albeit ones with side-effects, setting the
named variable appropriately). Thus they can be used as if-conditions,
and any post-checking or processing can occur in the then-statement.
- Updates all places where the macros were used.
- Updates Helgrind to use the *_CLO_* macros (it didn't use them).
- Updates Callgrind to use the *_CLO_* macros (it didn't use them), except
for the more esoteric option names (those with numbers in the option
name). This allowed getUInt() and getUWord() to be removed.
- Improves the cache option parsing in Cachegrind and Callgrind -- now uses
VG_(strtoll10)(), detects overflow, and is shorter.
- Uses INT instead of NUM in the macro names, to distinguish better vs. the
DBL macro.
- Removes VG_(atoll*) and the few remaining uses -- they're wretched
functions and VG_(strtoll*) should be used instead.
- Adds the VG_STREQN macro.
Advantages:
- Uniform CLO handling -- the *_CLO_* macros can be used everywhere. Thus
all options of one type (eg. numbers) are handled the same.
- Malformed numeric arguments (eg. --log-fd="foo") aren't accepted.
- You don't have to specify the lengths of any option strings anywhere (eg.
the 9 in the --log-fd example above). The use of a wrong number caused at
least one bug, in Massif.
- More concise -- removes 130 lines of code
I'd also like to change VG_BINT_CLO and VG_BHEX_CLO to abort if the given
value is outside the range -- the current silent truncation just seems
likely to cause confusion as much as anything. But I haven't done that in
this patch.
Thoughts?
Nick
|