From 83df0b67a14425c484d8dda42b53f3ff0b598894 Mon Sep 17 00:00:00 2001 From: njn Date: Wed, 25 Feb 2009 01:01:05 +0000 Subject: 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 --- massif/ms_main.c | 57 ++++++++++++++++++++------------------------------------ 1 file changed, 20 insertions(+), 37 deletions(-) (limited to 'massif') diff --git a/massif/ms_main.c b/massif/ms_main.c index 32b6eeca7..2d8c26a05 100644 --- a/massif/ms_main.c +++ b/massif/ms_main.c @@ -358,46 +358,45 @@ static Bool clo_heap = True; // word-sized type -- it ended up with a value of 4.2 billion. Sigh. static SSizeT clo_heap_admin = 8; static Bool clo_stacks = False; -static UInt clo_depth = 30; +static Int clo_depth = 30; static double clo_threshold = 1.0; // percentage static double clo_peak_inaccuracy = 1.0; // percentage -static UInt clo_time_unit = TimeI; -static UInt clo_detailed_freq = 10; -static UInt clo_max_snapshots = 100; +static Int clo_time_unit = TimeI; +static Int clo_detailed_freq = 10; +static Int clo_max_snapshots = 100; static Char* clo_massif_out_file = "massif.out.%p"; static XArray* args_for_massif; static Bool ms_process_cmd_line_option(Char* arg) { + Char* tmp_str; + // Remember the arg for later use. VG_(addToXA)(args_for_massif, &arg); - VG_BOOL_CLO(arg, "--heap", clo_heap) - else VG_BOOL_CLO(arg, "--stacks", clo_stacks) + if VG_BOOL_CLO(arg, "--heap", clo_heap) {} + else if 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) + else if VG_BINT_CLO(arg, "--heap-admin", clo_heap_admin, 0, 1024) {} + else if VG_BINT_CLO(arg, "--depth", clo_depth, 1, MAX_DEPTH) {} - else VG_DBL_CLO(arg, "--threshold", clo_threshold) + else if VG_DBL_CLO(arg, "--threshold", clo_threshold) {} - else VG_DBL_CLO(arg, "--peak-inaccuracy", clo_peak_inaccuracy) + else if VG_DBL_CLO(arg, "--peak-inaccuracy", clo_peak_inaccuracy) {} - else VG_NUM_CLO(arg, "--detailed-freq", clo_detailed_freq) - else VG_NUM_CLO(arg, "--max-snapshots", clo_max_snapshots) + else if VG_BINT_CLO(arg, "--detailed-freq", clo_detailed_freq, 1, 10000) {} + else if VG_BINT_CLO(arg, "--max-snapshots", clo_max_snapshots, 10, 1000) {} - else if (VG_CLO_STREQ(arg, "--time-unit=i")) clo_time_unit = TimeI; - else if (VG_CLO_STREQ(arg, "--time-unit=ms")) clo_time_unit = TimeMS; - else if (VG_CLO_STREQ(arg, "--time-unit=B")) clo_time_unit = TimeB; + else if VG_XACT_CLO(arg, "--time-unit=i", clo_time_unit, TimeI) {} + else if VG_XACT_CLO(arg, "--time-unit=ms", clo_time_unit, TimeMS) {} + else if VG_XACT_CLO(arg, "--time-unit=B", clo_time_unit, TimeB) {} - else if (VG_CLO_STREQN(11, arg, "--alloc-fn=")) { - Char* alloc_fn = &arg[11]; - VG_(addToXA)(alloc_fns, &alloc_fn); + else if VG_STR_CLO(arg, "--alloc-fn", tmp_str) { + VG_(addToXA)(alloc_fns, &tmp_str); } - else if (VG_CLO_STREQN(18, arg, "--massif-out-file=")) { - clo_massif_out_file = &arg[18]; - } + else if VG_STR_CLO(arg, "--massif-out-file", clo_massif_out_file) {} else return VG_(replacement_malloc_process_cmd_line_option)(arg); @@ -2164,26 +2163,10 @@ static void ms_post_clo_init(void) Int i; // Check options. - if (clo_heap_admin < 0 || clo_heap_admin > 1024) { - VG_(message)(Vg_UserMsg, "--heap-admin must be between 0 and 1024"); - VG_(err_bad_option)("--heap-admin"); - } - if (clo_depth < 1 || clo_depth > MAX_DEPTH) { - VG_(message)(Vg_UserMsg, "--depth must be between 1 and %d", MAX_DEPTH); - VG_(err_bad_option)("--depth"); - } if (clo_threshold < 0 || clo_threshold > 100) { VG_(message)(Vg_UserMsg, "--threshold must be between 0.0 and 100.0"); VG_(err_bad_option)("--threshold"); } - if (clo_detailed_freq < 1 || clo_detailed_freq > 10000) { - VG_(message)(Vg_UserMsg, "--detailed-freq must be between 1 and 10000"); - VG_(err_bad_option)("--detailed-freq"); - } - if (clo_max_snapshots < 10 || clo_max_snapshots > 1000) { - VG_(message)(Vg_UserMsg, "--max-snapshots must be between 10 and 1000"); - VG_(err_bad_option)("--max-snapshots"); - } // If we have --heap=no, set --heap-admin to zero, just to make sure we // don't accidentally use a non-zero heap-admin size somewhere. -- cgit v1.2.3