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 they are actually 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 commit:
- 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. And
malformed numeric arguments (eg. --log-fd=foo) aren't accepted. This also
means 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.
- 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.
- Changes VG_BINT_CLO and VG_BHEX_CLO to abort if the given value is outside
the range -- the current silent truncation is likely to cause confusion as
much as anything.
git-svn-id: svn://svn.valgrind.org/valgrind/trunk@9255 a5019735-40e9-0310-863c-91ae7b9d1cf9
diff --git a/cachegrind/cg_main.c b/cachegrind/cg_main.c
index 36ddbab..e356d0e 100644
--- a/cachegrind/cg_main.c
+++ b/cachegrind/cg_main.c
@@ -1645,50 +1645,48 @@
static void parse_cache_opt ( cache_t* cache, Char* opt )
{
- Int i = 0, i2, i3;
+ Long i1, i2, i3;
+ Char* endptr;
- // Option argument looks like "65536,2,64".
- // Find commas, replace with NULs to make three independent
- // strings, then extract numbers, put NULs back. Yuck.
- while (VG_(isdigit)(opt[i])) i++;
- if (',' == opt[i]) {
- opt[i++] = '\0';
- i2 = i;
- } else goto bad;
- while (VG_(isdigit)(opt[i])) i++;
- if (',' == opt[i]) {
- opt[i++] = '\0';
- i3 = i;
- } else goto bad;
- while (VG_(isdigit)(opt[i])) i++;
- if ('\0' != opt[i]) goto bad;
+ // Option argument looks like "65536,2,64". Extract them.
+ i1 = VG_(strtoll10)(opt, &endptr); if (*endptr != ',') goto bad;
+ i2 = VG_(strtoll10)(endptr+1, &endptr); if (*endptr != ',') goto bad;
+ i3 = VG_(strtoll10)(endptr+1, &endptr); if (*endptr != '\0') goto bad;
- cache->size = (Int)VG_(atoll)(opt);
- cache->assoc = (Int)VG_(atoll)(opt + i2);
- cache->line_size = (Int)VG_(atoll)(opt + i3);
+ // Check for overflow.
+ cache->size = (Int)i1;
+ cache->assoc = (Int)i2;
+ cache->line_size = (Int)i3;
+ if (cache->size != i1) goto overflow;
+ if (cache->assoc != i2) goto overflow;
+ if (cache->line_size != i3) goto overflow;
- opt[i2-1] = ',';
- opt[i3-1] = ',';
return;
+ overflow:
+ VG_(message)(Vg_UserMsg,
+ "one of the cache parameters was too large and overflowed\n");
bad:
+ // XXX: this omits the "--I1/D1/L2=" part from the message, but that's
+ // not a big deal.
VG_(err_bad_option)(opt);
}
static Bool cg_process_cmd_line_option(Char* arg)
{
+ Char* tmp_str;
+
// 5 is length of "--I1="
- if (VG_CLO_STREQN(5, arg, "--I1="))
- parse_cache_opt(&clo_I1_cache, &arg[5]);
- else if (VG_CLO_STREQN(5, arg, "--D1="))
- parse_cache_opt(&clo_D1_cache, &arg[5]);
- else if (VG_CLO_STREQN(5, arg, "--L2="))
- parse_cache_opt(&clo_L2_cache, &arg[5]);
- else if (VG_CLO_STREQN(22, arg, "--cachegrind-out-file=")) {
- clo_cachegrind_out_file = &arg[22];
- }
- else VG_BOOL_CLO(arg, "--cache-sim", clo_cache_sim)
- else VG_BOOL_CLO(arg, "--branch-sim", clo_branch_sim)
+ if VG_STR_CLO(arg, "--I1", tmp_str)
+ parse_cache_opt(&clo_I1_cache, tmp_str);
+ else if VG_STR_CLO(arg, "--D1", tmp_str)
+ parse_cache_opt(&clo_D1_cache, tmp_str);
+ else if VG_STR_CLO(arg, "--L2", tmp_str)
+ parse_cache_opt(&clo_L2_cache, tmp_str);
+
+ else if VG_STR_CLO( arg, "--cachegrind-out-file", clo_cachegrind_out_file) {}
+ else if VG_BOOL_CLO(arg, "--cache-sim", clo_cache_sim) {}
+ else if VG_BOOL_CLO(arg, "--branch-sim", clo_branch_sim) {}
else
return False;