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/callgrind/sim.c b/callgrind/sim.c
index 5208a72..d6c1211 100644
--- a/callgrind/sim.c
+++ b/callgrind/sim.c
@@ -1506,40 +1506,33 @@
);
}
-static void parse_opt ( cache_t* cache, char* orig_opt, int opt_len )
+static void parse_opt ( cache_t* cache, char* opt )
{
- int i1, i2, i3;
- int i;
- char *opt = VG_(strdup)("cl.sim.po.1", orig_opt);
+ Long i1, i2, i3;
+ Char* endptr;
- i = i1 = opt_len;
+ // 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;
- /* Option looks like "--I1=65536,2,64".
- * Find commas, replace with NULs to make three independent
- * strings, then extract numbers. 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;
-
- cache->size = (Int)VG_(atoll)(opt + i1);
- cache->assoc = (Int)VG_(atoll)(opt + i2);
- cache->line_size = (Int)VG_(atoll)(opt + i3);
-
- VG_(free)(opt);
+ // 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;
return;
+ overflow:
+ VG_(message)(Vg_UserMsg,
+ "one of the cache parameters was too large and overflowed\n");
bad:
- VG_(err_bad_option)(orig_opt);
+ // XXX: this omits the "--I1/D1/L2=" part from the message, but that's
+ // not a big deal.
+ VG_(err_bad_option)(opt);
}
/* Check for command line option for cache configuration.
@@ -1549,36 +1542,25 @@
*/
static Bool cachesim_parse_opt(Char* arg)
{
- if (0 == VG_(strcmp)(arg, "--simulate-wb=yes"))
- clo_simulate_writeback = True;
- else if (0 == VG_(strcmp)(arg, "--simulate-wb=no"))
- clo_simulate_writeback = False;
+ Char* tmp_str;
- else if (0 == VG_(strcmp)(arg, "--simulate-hwpref=yes"))
- clo_simulate_hwpref = True;
- else if (0 == VG_(strcmp)(arg, "--simulate-hwpref=no"))
- clo_simulate_hwpref = False;
+ if VG_BOOL_CLO(arg, "--simulate-wb", clo_simulate_writeback) {}
+ else if VG_BOOL_CLO(arg, "--simulate-hwpref", clo_simulate_hwpref) {}
+ else if VG_BOOL_CLO(arg, "--simulate-sectors", clo_simulate_sectors) {}
- else if (0 == VG_(strcmp)(arg, "--simulate-sectors=yes"))
- clo_simulate_sectors = True;
- else if (0 == VG_(strcmp)(arg, "--simulate-sectors=no"))
- clo_simulate_sectors = False;
+ else if VG_BOOL_CLO(arg, "--cacheuse", clo_collect_cacheuse) {
+ if (clo_collect_cacheuse) {
+ /* Use counters only make sense with fine dumping */
+ CLG_(clo).dump_instr = True;
+ }
+ }
- else if (0 == VG_(strcmp)(arg, "--cacheuse=yes")) {
- clo_collect_cacheuse = True;
- /* Use counters only make sense with fine dumping */
- CLG_(clo).dump_instr = True;
- }
- else if (0 == VG_(strcmp)(arg, "--cacheuse=no"))
- clo_collect_cacheuse = False;
-
- /* 5 is length of "--I1=" */
- else if (0 == VG_(strncmp)(arg, "--I1=", 5))
- parse_opt(&clo_I1_cache, arg, 5);
- else if (0 == VG_(strncmp)(arg, "--D1=", 5))
- parse_opt(&clo_D1_cache, arg, 5);
- else if (0 == VG_(strncmp)(arg, "--L2=", 5))
- parse_opt(&clo_L2_cache, arg, 5);
+ else if VG_STR_CLO(arg, "--I1", tmp_str)
+ parse_opt(&clo_I1_cache, tmp_str);
+ else if VG_STR_CLO(arg, "--D1", tmp_str)
+ parse_opt(&clo_D1_cache, tmp_str);
+ else if VG_STR_CLO(arg, "--L2", tmp_str)
+ parse_opt(&clo_L2_cache, tmp_str);
else
return False;