From 11f94ef626165beac82bac7a41aa05db431a1c39 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 6 Jul 2010 04:05:23 +0000 Subject: [PATCH] Make error messages at start-up more consistent. Every line of such messages now begin with "valgrind: ", and they're more often printed before the preamble. This required introducing a new message kind, Vg_FailMsg, and functions VG_(fmsg) and VG_(fmsg_bad_option), and removing VG_(err_bad_option). Where we used to have horrible output like this: [ocean:~/grind/ws2] vg5 --tool=massif --threshold=101 date ==31877== Massif, a heap profiler ==31877== Copyright (C) 2003-2010, and GNU GPL'd, by Nicholas Nethercote ==31877== Using Valgrind-3.6.0.SVN and LibVEX; rerun with -h for copyright info ==31877== Command: date ==31877== ==31877== --threshold must be between 0.0 and 100.0 valgrind: Bad option '--threshold'; aborting. valgrind: Use --help for more information. We now have nice output like this: [ocean:~/grind/ws2] vg2 --tool=massif --threshold=101 date valgrind: Bad option: --threshold=101 valgrind: --threshold must be between 0.0 and 100.0 valgrind: Use --help for more information or consult the user manual. git-svn-id: svn://svn.valgrind.org/valgrind/trunk@11209 --- cachegrind/cg_main.c | 88 ++++++----- callgrind/sim.c | 89 +++++------ coregrind/m_libcprint.c | 107 ++++++++++--- coregrind/m_main.c | 149 +++++++----------- coregrind/m_options.c | 71 +++------ .../m_replacemalloc/replacemalloc_core.c | 9 +- coregrind/m_ume/main.c | 15 +- coregrind/pub_core_libcprint.h | 10 ++ coregrind/pub_core_options.h | 12 -- include/pub_tool_libcprint.h | 111 +++++++------ include/pub_tool_options.h | 24 +-- include/pub_tool_tooliface.h | 15 +- massif/ms_main.c | 15 +- memcheck/mc_main.c | 13 +- none/tests/cmdline2.stdout.exp | 2 +- none/tests/cmdline4.stderr.exp | 4 +- 16 files changed, 371 insertions(+), 363 deletions(-) diff --git a/cachegrind/cg_main.c b/cachegrind/cg_main.c index e858ef5cac..cb0af56821 100644 --- a/cachegrind/cg_main.c +++ b/cachegrind/cg_main.c @@ -1191,45 +1191,40 @@ static cache_t clo_I1_cache = UNDEFINED_CACHE; static cache_t clo_D1_cache = UNDEFINED_CACHE; static cache_t clo_L2_cache = UNDEFINED_CACHE; -/* Checks cache config is ok; makes it so if not. */ -static -void check_cache(cache_t* cache, Char *name) +// Checks cache config is ok. Returns NULL if ok, or a pointer to an error +// string otherwise. +static Char* check_cache(cache_t* cache) { - /* Simulator requires line size and set count to be powers of two */ - if (( cache->size % (cache->line_size * cache->assoc) != 0) || - (-1 == VG_(log2)(cache->size/cache->line_size/cache->assoc))) { - VG_(umsg)("error: %s set count not a power of two; aborting.\n", name); - VG_(exit)(1); + // Simulator requires set count to be a power of two. + if ((cache->size % (cache->line_size * cache->assoc) != 0) || + (-1 == VG_(log2)(cache->size/cache->line_size/cache->assoc))) + { + return "Cache set count is not a power of two.\n"; } + // Simulator requires line size to be a power of two. if (-1 == VG_(log2)(cache->line_size)) { - VG_(umsg)("error: %s line size of %dB not a power of two; aborting.\n", - name, cache->line_size); - VG_(exit)(1); + return "Cache line size is not a power of two.\n"; } // Then check line size >= 16 -- any smaller and a single instruction could // straddle three cache lines, which breaks a simulation assertion and is // stupid anyway. if (cache->line_size < MIN_LINE_SIZE) { - VG_(umsg)("error: %s line size of %dB too small; aborting.\n", - name, cache->line_size); - VG_(exit)(1); + return "Cache line size is too small.\n"; } /* Then check cache size > line size (causes seg faults if not). */ if (cache->size <= cache->line_size) { - VG_(umsg)("error: %s cache size of %dB <= line size of %dB; aborting.\n", - name, cache->size, cache->line_size); - VG_(exit)(1); + return "Cache size <= line size.\n"; } /* Then check assoc <= (size / line size) (seg faults otherwise). */ if (cache->assoc > (cache->size / cache->line_size)) { - VG_(umsg)("warning: %s associativity > (size / line size); aborting.\n", - name); - VG_(exit)(1); + return "Cache associativity > (size / line size).\n"; } + + return NULL; } static @@ -1237,27 +1232,29 @@ void configure_caches(cache_t* I1c, cache_t* D1c, cache_t* L2c) { #define DEFINED(L) (-1 != L.size || -1 != L.assoc || -1 != L.line_size) - Int n_clos = 0; + Char* checkRes; // Count how many were defined on the command line. - if (DEFINED(clo_I1_cache)) { n_clos++; } - if (DEFINED(clo_D1_cache)) { n_clos++; } - if (DEFINED(clo_L2_cache)) { n_clos++; } + Bool all_caches_clo_defined = + (DEFINED(clo_I1_cache) && + DEFINED(clo_D1_cache) && + DEFINED(clo_L2_cache)); // Set the cache config (using auto-detection, if supported by the - // architecture) - VG_(configure_caches)( I1c, D1c, L2c, (3 == n_clos) ); + // architecture). + VG_(configure_caches)( I1c, D1c, L2c, all_caches_clo_defined ); - // Then replace with any defined on the command line. + // Check the default/auto-detected values. + checkRes = check_cache(I1c); tl_assert(!checkRes); + checkRes = check_cache(D1c); tl_assert(!checkRes); + checkRes = check_cache(L2c); tl_assert(!checkRes); + + // Then replace with any defined on the command line. (Already checked in + // parse_cache_opt().) if (DEFINED(clo_I1_cache)) { *I1c = clo_I1_cache; } if (DEFINED(clo_D1_cache)) { *D1c = clo_D1_cache; } if (DEFINED(clo_L2_cache)) { *L2c = clo_L2_cache; } - // Then check values and fix if not acceptable. - check_cache(I1c, "I1"); - check_cache(D1c, "D1"); - check_cache(L2c, "L2"); - if (VG_(clo_verbosity) >= 2) { VG_(umsg)("Cache configuration used:\n"); VG_(umsg)(" I1: %dB, %d-way, %dB lines\n", @@ -1671,13 +1668,14 @@ void cg_discard_superblock_info ( Addr64 orig_addr64, VexGuestExtents vge ) /*--- Command line processing ---*/ /*--------------------------------------------------------------------*/ -static void parse_cache_opt ( cache_t* cache, Char* opt ) +static void parse_cache_opt ( cache_t* cache, Char* opt, Char* optval ) { Long i1, i2, i3; Char* endptr; + Char* checkRes; // Option argument looks like "65536,2,64". Extract them. - i1 = VG_(strtoll10)(opt, &endptr); if (*endptr != ',') goto bad; + i1 = VG_(strtoll10)(optval, &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; @@ -1689,14 +1687,20 @@ static void parse_cache_opt ( cache_t* cache, Char* opt ) if (cache->assoc != i2) goto overflow; if (cache->line_size != i3) goto overflow; + checkRes = check_cache(cache); + if (checkRes) { + VG_(fmsg)("%s", checkRes); + goto bad; + } + return; - overflow: - VG_(umsg)("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); + VG_(fmsg_bad_option)(opt, ""); + + overflow: + VG_(fmsg_bad_option)(opt, + "One of the cache parameters was too large and overflowed.\n"); } static Bool cg_process_cmd_line_option(Char* arg) @@ -1705,11 +1709,11 @@ static Bool cg_process_cmd_line_option(Char* arg) // 5 is length of "--I1=" if VG_STR_CLO(arg, "--I1", tmp_str) - parse_cache_opt(&clo_I1_cache, tmp_str); + parse_cache_opt(&clo_I1_cache, arg, tmp_str); else if VG_STR_CLO(arg, "--D1", tmp_str) - parse_cache_opt(&clo_D1_cache, tmp_str); + parse_cache_opt(&clo_D1_cache, arg, tmp_str); else if VG_STR_CLO(arg, "--L2", tmp_str) - parse_cache_opt(&clo_L2_cache, tmp_str); + parse_cache_opt(&clo_L2_cache, arg, 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) {} diff --git a/callgrind/sim.c b/callgrind/sim.c index 0841d2c657..cb41d57d1e 100644 --- a/callgrind/sim.c +++ b/callgrind/sim.c @@ -1271,49 +1271,40 @@ static cache_t clo_D1_cache = UNDEFINED_CACHE; static cache_t clo_L2_cache = UNDEFINED_CACHE; -/* Checks cache config is ok; makes it so if not. */ -static -void check_cache(cache_t* cache, Char *name) +// Checks cache config is ok. Returns NULL if ok, or a pointer to an error +// string otherwise. +static Char* check_cache(cache_t* cache) { - /* Simulator requires line size and set count to be powers of two */ + // Simulator requires line size and set count to be powers of two. if (( cache->size % (cache->line_size * cache->assoc) != 0) || - (-1 == VG_(log2)(cache->size/cache->line_size/cache->assoc))) { - VG_(message)(Vg_UserMsg, - "error: %s set count not a power of two; aborting.\n", - name); + (-1 == VG_(log2)(cache->size/cache->line_size/cache->assoc))) + { + return "Cache set count is not a power of two.\n"; } + // Simulator requires line size to be a power of two. if (-1 == VG_(log2)(cache->line_size)) { - VG_(message)(Vg_UserMsg, - "error: %s line size of %dB not a power of two; aborting.\n", - name, cache->line_size); - VG_(exit)(1); + return "Cache line size is not a power of two.\n"; } // Then check line size >= 16 -- any smaller and a single instruction could // straddle three cache lines, which breaks a simulation assertion and is // stupid anyway. if (cache->line_size < MIN_LINE_SIZE) { - VG_(message)(Vg_UserMsg, - "error: %s line size of %dB too small; aborting.\n", - name, cache->line_size); - VG_(exit)(1); + return "Cache line size is too small.\n"; } /* Then check cache size > line size (causes seg faults if not). */ if (cache->size <= cache->line_size) { - VG_(message)(Vg_UserMsg, - "error: %s cache size of %dB <= line size of %dB; aborting.\n", - name, cache->size, cache->line_size); - VG_(exit)(1); + return "Cache size <= line size.\n"; } /* Then check assoc <= (size / line size) (seg faults otherwise). */ if (cache->assoc > (cache->size / cache->line_size)) { - VG_(message)(Vg_UserMsg, - "warning: %s associativity > (size / line size); aborting.\n", name); - VG_(exit)(1); + return "Cache associativity > (size / line size).\n"; } + + return NULL; } static @@ -1321,27 +1312,27 @@ void configure_caches(cache_t* I1c, cache_t* D1c, cache_t* L2c) { #define DEFINED(L) (-1 != L.size || -1 != L.assoc || -1 != L.line_size) - Int n_clos = 0; + Char* checkRes; - // Count how many were defined on the command line. - if (DEFINED(clo_I1_cache)) { n_clos++; } - if (DEFINED(clo_D1_cache)) { n_clos++; } - if (DEFINED(clo_L2_cache)) { n_clos++; } + Bool all_caches_clo_defined = + (DEFINED(clo_I1_cache) && + DEFINED(clo_D1_cache) && + DEFINED(clo_L2_cache)); // Set the cache config (using auto-detection, if supported by the - // architecture) - VG_(configure_caches)( I1c, D1c, L2c, (3 == n_clos) ); + // architecture). + VG_(configure_caches)( I1c, D1c, L2c, all_caches_clo_defined ); + + // Check the default/auto-detected values. + checkRes = check_cache(I1c); tl_assert(!checkRes); + checkRes = check_cache(D1c); tl_assert(!checkRes); + checkRes = check_cache(L2c); tl_assert(!checkRes); // Then replace with any defined on the command line. if (DEFINED(clo_I1_cache)) { *I1c = clo_I1_cache; } if (DEFINED(clo_D1_cache)) { *D1c = clo_D1_cache; } if (DEFINED(clo_L2_cache)) { *L2c = clo_L2_cache; } - // Then check values and fix if not acceptable. - check_cache(I1c, "I1"); - check_cache(D1c, "D1"); - check_cache(L2c, "L2"); - if (VG_(clo_verbosity) > 1) { VG_(message)(Vg_UserMsg, "Cache configuration used:\n"); VG_(message)(Vg_UserMsg, " I1: %dB, %d-way, %dB lines\n", @@ -1503,13 +1494,14 @@ void cachesim_print_opts(void) ); } -static void parse_opt ( cache_t* cache, char* opt ) +static void parse_opt ( cache_t* cache, char* opt, Char* optval ) { Long i1, i2, i3; Char* endptr; + Char* checkRes; // Option argument looks like "65536,2,64". Extract them. - i1 = VG_(strtoll10)(opt, &endptr); if (*endptr != ',') goto bad; + i1 = VG_(strtoll10)(optval, &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; @@ -1521,15 +1513,20 @@ static void parse_opt ( cache_t* cache, char* opt ) if (cache->assoc != i2) goto overflow; if (cache->line_size != i3) goto overflow; + checkRes = check_cache(cache); + if (checkRes) { + VG_(fmsg)("%s", checkRes); + goto bad; + } + 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); + VG_(fmsg_bad_option)(opt, ""); + + overflow: + VG_(fmsg_bad_option)(opt, + "One of the cache parameters was too large and overflowed.\n"); } /* Check for command line option for cache configuration. @@ -1553,11 +1550,11 @@ static Bool cachesim_parse_opt(Char* arg) } else if VG_STR_CLO(arg, "--I1", tmp_str) - parse_opt(&clo_I1_cache, tmp_str); + parse_opt(&clo_I1_cache, arg, tmp_str); else if VG_STR_CLO(arg, "--D1", tmp_str) - parse_opt(&clo_D1_cache, tmp_str); + parse_opt(&clo_D1_cache, arg, tmp_str); else if VG_STR_CLO(arg, "--L2", tmp_str) - parse_opt(&clo_L2_cache, tmp_str); + parse_opt(&clo_L2_cache, arg, tmp_str); else return False; diff --git a/coregrind/m_libcprint.c b/coregrind/m_libcprint.c index 3a545049ab..41171267d4 100644 --- a/coregrind/m_libcprint.c +++ b/coregrind/m_libcprint.c @@ -400,14 +400,6 @@ static void add_to__vmessage_buf ( HChar c, void *p ) HChar ch; Int i, depth; - switch (b->kind) { - case Vg_UserMsg: ch = '='; break; - case Vg_DebugMsg: ch = '-'; break; - case Vg_DebugExtraMsg: ch = '+'; break; - case Vg_ClientMsg: ch = '*'; break; - default: ch = '?'; break; - } - // Print one '>' in front of the messages for each level of // self-hosting being performed. depth = RUNNING_ON_VALGRIND; @@ -417,25 +409,47 @@ static void add_to__vmessage_buf ( HChar c, void *p ) b->buf[b->buf_used++] = '>'; } - b->buf[b->buf_used++] = ch; - b->buf[b->buf_used++] = ch; - - if (VG_(clo_time_stamp)) { - VG_(memset)(tmp, 0, sizeof(tmp)); - VG_(elapsed_wallclock_time)(tmp); + if (Vg_FailMsg == b->kind) { + // "valgrind: " prefix. + b->buf[b->buf_used++] = 'v'; + b->buf[b->buf_used++] = 'a'; + b->buf[b->buf_used++] = 'l'; + b->buf[b->buf_used++] = 'g'; + b->buf[b->buf_used++] = 'r'; + b->buf[b->buf_used++] = 'i'; + b->buf[b->buf_used++] = 'n'; + b->buf[b->buf_used++] = 'd'; + b->buf[b->buf_used++] = ':'; + b->buf[b->buf_used++] = ' '; + } else { + switch (b->kind) { + case Vg_UserMsg: ch = '='; break; + case Vg_DebugMsg: ch = '-'; break; + case Vg_DebugExtraMsg: ch = '+'; break; + case Vg_ClientMsg: ch = '*'; break; + default: ch = '?'; break; + } + + b->buf[b->buf_used++] = ch; + b->buf[b->buf_used++] = ch; + + if (VG_(clo_time_stamp)) { + VG_(memset)(tmp, 0, sizeof(tmp)); + VG_(elapsed_wallclock_time)(tmp); + tmp[sizeof(tmp)-1] = 0; + for (i = 0; tmp[i]; i++) + b->buf[b->buf_used++] = tmp[i]; + } + + VG_(sprintf)(tmp, "%d", VG_(getpid)()); tmp[sizeof(tmp)-1] = 0; for (i = 0; tmp[i]; i++) b->buf[b->buf_used++] = tmp[i]; - } - VG_(sprintf)(tmp, "%d", VG_(getpid)()); - tmp[sizeof(tmp)-1] = 0; - for (i = 0; tmp[i]; i++) - b->buf[b->buf_used++] = tmp[i]; - - b->buf[b->buf_used++] = ch; - b->buf[b->buf_used++] = ch; - b->buf[b->buf_used++] = ' '; + b->buf[b->buf_used++] = ch; + b->buf[b->buf_used++] = ch; + b->buf[b->buf_used++] = ' '; + } /* We can't possibly have stuffed 96 chars in merely as a result of making the preamble (can we?) */ @@ -503,7 +517,36 @@ UInt VG_(message) ( VgMsgKind kind, const HChar* format, ... ) return count; } +static void revert_to_stderr ( void ) +{ + VG_(log_output_sink).fd = 2; /* stderr */ + VG_(log_output_sink).is_socket = False; +} + /* VG_(message) variants with hardwired first argument. */ + +UInt VG_(fmsg) ( const HChar* format, ... ) +{ + UInt count; + va_list vargs; + va_start(vargs,format); + count = VG_(vmessage) ( Vg_FailMsg, format, vargs ); + va_end(vargs); + return count; +} + +void VG_(fmsg_bad_option) ( HChar* opt, const HChar* format, ... ) +{ + va_list vargs; + va_start(vargs,format); + revert_to_stderr(); + VG_(message) (Vg_FailMsg, "Bad option: %s\n", opt); + VG_(vmessage)(Vg_FailMsg, format, vargs ); + VG_(message) (Vg_FailMsg, "Use --help for more information or consult the user manual.\n"); + VG_(exit)(1); + va_end(vargs); +} + UInt VG_(umsg) ( const HChar* format, ... ) { UInt count; @@ -543,6 +586,24 @@ void VG_(message_flush) ( void ) b->buf_used = 0; } +__attribute__((noreturn)) +void VG_(err_missing_prog) ( void ) +{ + revert_to_stderr(); + VG_(fmsg)("no program specified\n"); + VG_(fmsg)("Use --help for more information.\n"); + VG_(exit)(1); +} + +__attribute__((noreturn)) +void VG_(err_config_error) ( Char* msg ) +{ + revert_to_stderr(); + VG_(fmsg)("Startup or configuration error:\n %s\n", msg); + VG_(fmsg)("Unable to start up properly. Giving up.\n"); + VG_(exit)(1); +} + /*--------------------------------------------------------------------*/ /*--- end ---*/ diff --git a/coregrind/m_main.c b/coregrind/m_main.c index c57a7fd520..bae04633c9 100644 --- a/coregrind/m_main.c +++ b/coregrind/m_main.c @@ -177,8 +177,8 @@ static void usage_NORETURN ( Bool debug_help ) Char* usage2 = "\n" " debugging options for all Valgrind tools:\n" -" --stats=no|yes show tool and core statistics [no]\n" " -d show verbose debugging output\n" +" --stats=no|yes show tool and core statistics [no]\n" " --sanity-level= level of sanity checking to do [1]\n" " --trace-flags= show generated code? (X = 0|1) [00000000]\n" " --profile-flags= ditto, but for profiling (X = 0|1) [00000000]\n" @@ -542,11 +542,9 @@ void main_process_cmd_line_options ( /*OUT*/Bool* logging_to_fd, else if VG_STR_CLO(arg, "--suppressions", tmp_str) { if (VG_(clo_n_suppressions) >= VG_CLO_MAX_SFILES) { - VG_(message)(Vg_UserMsg, - "Too many suppression files specified.\n"); - VG_(message)(Vg_UserMsg, - "Increase VG_CLO_MAX_SFILES and recompile.\n"); - VG_(err_bad_option)(arg); + VG_(fmsg_bad_option)(arg, + "Too many suppression files specified.\n" + "Increase VG_CLO_MAX_SFILES and recompile.\n"); } VG_(clo_suppressions)[VG_(clo_n_suppressions)] = tmp_str; VG_(clo_n_suppressions)++; @@ -554,11 +552,9 @@ void main_process_cmd_line_options ( /*OUT*/Bool* logging_to_fd, else if VG_STR_CLO(arg, "--require-text-symbol", tmp_str) { if (VG_(clo_n_req_tsyms) >= VG_CLO_MAX_REQ_TSYMS) { - VG_(message)(Vg_UserMsg, - "Too many --require-text-symbol= specifications.\n"); - VG_(message)(Vg_UserMsg, - "Increase VG_CLO_MAX_REQ_TSYMS and recompile.\n"); - VG_(err_bad_option)(arg); + VG_(fmsg_bad_option)(arg, + "Too many --require-text-symbol= specifications.\n" + "Increase VG_CLO_MAX_REQ_TSYMS and recompile.\n"); } /* String needs to be of the form C?*C?*, where C is any character, but is the same both times. Having it in this @@ -577,9 +573,8 @@ void main_process_cmd_line_options ( /*OUT*/Bool* logging_to_fd, ok = VG_(string_match)(patt, tmp_str); } if (!ok) { - VG_(message)(Vg_UserMsg, - "Invalid --require-text-symbol= specification.\n"); - VG_(err_bad_option)(arg); + VG_(fmsg_bad_option)(arg, + "Invalid --require-text-symbol= specification.\n"); } VG_(clo_req_tsyms)[VG_(clo_n_req_tsyms)] = tmp_str; VG_(clo_n_req_tsyms)++; @@ -590,17 +585,15 @@ void main_process_cmd_line_options ( /*OUT*/Bool* logging_to_fd, Int j; if (8 != VG_(strlen)(tmp_str)) { - VG_(message)(Vg_UserMsg, - "--trace-flags argument must have 8 digits\n"); - VG_(err_bad_option)(arg); + VG_(fmsg_bad_option)(arg, + "--trace-flags argument must have 8 digits\n"); } for (j = 0; j < 8; j++) { if ('0' == tmp_str[j]) { /* do nothing */ } else if ('1' == tmp_str[j]) VG_(clo_trace_flags) |= (1 << (7-j)); else { - VG_(message)(Vg_UserMsg, "--trace-flags argument can only " - "contain 0s and 1s\n"); - VG_(err_bad_option)(arg); + VG_(fmsg_bad_option)(arg, + "--trace-flags argument can only contain 0s and 1s\n"); } } } @@ -610,17 +603,15 @@ void main_process_cmd_line_options ( /*OUT*/Bool* logging_to_fd, Int j; if (8 != VG_(strlen)(tmp_str)) { - VG_(message)(Vg_UserMsg, - "--profile-flags argument must have 8 digits\n"); - VG_(err_bad_option)(arg); + VG_(fmsg_bad_option)(arg, + "--profile-flags argument must have 8 digits\n"); } for (j = 0; j < 8; j++) { if ('0' == tmp_str[j]) { /* do nothing */ } else if ('1' == tmp_str[j]) VG_(clo_profile_flags) |= (1 << (7-j)); else { - VG_(message)(Vg_UserMsg, "--profile-flags argument can only " - "contain 0s and 1s\n"); - VG_(err_bad_option)(arg); + VG_(fmsg_bad_option)(arg, + "--profile-flags argument can only contain 0s and 1s\n"); } } } @@ -636,7 +627,7 @@ void main_process_cmd_line_options ( /*OUT*/Bool* logging_to_fd, else if ( ! VG_(needs).command_line_options || ! VG_TDICT_CALL(tool_process_cmd_line_option, arg) ) { - VG_(err_bad_option)(arg); + VG_(fmsg_bad_option)(arg, ""); } } @@ -659,20 +650,17 @@ void main_process_cmd_line_options ( /*OUT*/Bool* logging_to_fd, if (VG_(clo_gen_suppressions) > 0 && !VG_(needs).core_errors && !VG_(needs).tool_errors) { - VG_(message)(Vg_UserMsg, - "Can't use --gen-suppressions= with this tool,\n"); - VG_(message)(Vg_UserMsg, - "as it doesn't generate errors.\n"); - VG_(err_bad_option)("--gen-suppressions="); + VG_(fmsg_bad_option)("--gen-suppressions=yes", + "Can't use --gen-suppressions= with %s\n" + "because it doesn't generate errors.\n", VG_(details).name); } /* If XML output is requested, check that the tool actually supports it. */ if (VG_(clo_xml) && !VG_(needs).xml_output) { VG_(clo_xml) = False; - VG_(message)(Vg_UserMsg, + VG_(fmsg_bad_option)("--xml=yes", "%s does not support XML output.\n", VG_(details).name); - VG_(err_bad_option)("--xml=yes"); /*NOTREACHED*/ } @@ -693,33 +681,28 @@ void main_process_cmd_line_options ( /*OUT*/Bool* logging_to_fd, (--gen-suppressions=all is still OK since we don't need any user interaction in this case.) */ if (VG_(clo_gen_suppressions) == 1) { - VG_(umsg)( - "When --xml=yes is specified, only --gen-suppressions=no\n" - "or --gen-suppressions=all are allowed, but not " + VG_(fmsg_bad_option)( + "--xml=yes together with --gen-suppressions=yes", + "When --xml=yes is specified, --gen-suppressions=no\n" + "or --gen-suppressions=all is allowed, but not " "--gen-suppressions=yes.\n"); - /* FIXME: this is really a misuse of VG_(err_bad_option). */ - VG_(err_bad_option)( - "--xml=yes together with --gen-suppressions=yes"); } /* We can't allow DB attaching (or we maybe could, but results could be chaotic ..) since it requires user input. Hence disallow. */ if (VG_(clo_db_attach)) { - VG_(umsg)("--db-attach=yes is not allowed in XML mode,\n" - "as it would require user input.\n"); - /* FIXME: this is really a misuse of VG_(err_bad_option). */ - VG_(err_bad_option)( - "--xml=yes together with --db-attach=yes"); + VG_(fmsg_bad_option)( + "--xml=yes together with --db-attach=yes", + "--db-attach=yes is not allowed with --xml=yes\n" + "because it would require user input.\n"); } /* Disallow dump_error in XML mode; sounds like a recipe for chaos. No big deal; dump_error is a flag for debugging V itself. */ if (VG_(clo_dump_error) > 0) { - /* FIXME: this is really a misuse of VG_(err_bad_option). */ - VG_(err_bad_option)( - "--xml=yes together with --dump-error="); + VG_(fmsg_bad_option)("--xml=yes together with --dump-error", ""); } /* Disable error limits (this might be a bad idea!) */ @@ -777,11 +760,9 @@ void main_process_cmd_line_options ( /*OUT*/Bool* logging_to_fd, tmp_log_fd = sr_Res(sres); VG_(clo_log_fname_expanded) = logfilename; } else { - VG_(message)(Vg_UserMsg, - "Can't create log file '%s' (%s); giving up!\n", - logfilename, VG_(strerror)(sr_Err(sres))); - VG_(err_bad_option)( - "--log-file= (didn't work out for some reason.)"); + VG_(fmsg)("can't create log file '%s': %s\n", + logfilename, VG_(strerror)(sr_Err(sres))); + VG_(exit)(1); /*NOTREACHED*/ } break; @@ -792,23 +773,16 @@ void main_process_cmd_line_options ( /*OUT*/Bool* logging_to_fd, vg_assert(VG_(strlen)(log_fsname_unexpanded) <= 900); /* paranoia */ tmp_log_fd = VG_(connect_via_socket)( log_fsname_unexpanded ); if (tmp_log_fd == -1) { - VG_(message)(Vg_UserMsg, - "Invalid --log-socket=ipaddr or " - "--log-socket=ipaddr:port spec\n"); - VG_(message)(Vg_UserMsg, - "of '%s'; giving up!\n", log_fsname_unexpanded ); - VG_(err_bad_option)( - "--log-socket="); + VG_(fmsg)("Invalid --log-socket spec of '%s'\n", + log_fsname_unexpanded); + VG_(exit)(1); /*NOTREACHED*/ } if (tmp_log_fd == -2) { - VG_(message)(Vg_UserMsg, - "valgrind: failed to connect to logging server '%s'.\n", - log_fsname_unexpanded ); - VG_(message)(Vg_UserMsg, - "Log messages will sent to stderr instead.\n" ); - VG_(message)(Vg_UserMsg, - "\n" ); + VG_(umsg)("failed to connect to logging server '%s'.\n" + "Log messages will sent to stderr instead.\n", + log_fsname_unexpanded ); + /* We don't change anything here. */ vg_assert(VG_(log_output_sink).fd == 2); tmp_log_fd = 2; @@ -848,11 +822,9 @@ void main_process_cmd_line_options ( /*OUT*/Bool* logging_to_fd, *xml_fname_unexpanded = VG_(strdup)( "main.mpclo.2", xml_fsname_unexpanded ); } else { - VG_(message)(Vg_UserMsg, - "Can't create XML file '%s' (%s); giving up!\n", - xmlfilename, VG_(strerror)(sr_Err(sres))); - VG_(err_bad_option)( - "--xml-file= (didn't work out for some reason.)"); + VG_(fmsg)("can't create XML file '%s': %s\n", + xmlfilename, VG_(strerror)(sr_Err(sres))); + VG_(exit)(1); /*NOTREACHED*/ } break; @@ -863,23 +835,15 @@ void main_process_cmd_line_options ( /*OUT*/Bool* logging_to_fd, vg_assert(VG_(strlen)(xml_fsname_unexpanded) <= 900); /* paranoia */ tmp_xml_fd = VG_(connect_via_socket)( xml_fsname_unexpanded ); if (tmp_xml_fd == -1) { - VG_(message)(Vg_UserMsg, - "Invalid --xml-socket=ipaddr or " - "--xml-socket=ipaddr:port spec\n"); - VG_(message)(Vg_UserMsg, - "of '%s'; giving up!\n", xml_fsname_unexpanded ); - VG_(err_bad_option)( - "--xml-socket="); + VG_(fmsg)("Invalid --xml-socket spec of '%s'\n", + xml_fsname_unexpanded ); + VG_(exit)(1); /*NOTREACHED*/ } if (tmp_xml_fd == -2) { - VG_(message)(Vg_UserMsg, - "valgrind: failed to connect to XML logging server '%s'.\n", - xml_fsname_unexpanded ); - VG_(message)(Vg_UserMsg, - "XML output will sent to stderr instead.\n" ); - VG_(message)(Vg_UserMsg, - "\n" ); + VG_(umsg)("failed to connect to XML logging server '%s'.\n" + "XML output will sent to stderr instead.\n", + xml_fsname_unexpanded); /* We don't change anything here. */ vg_assert(VG_(xml_output_sink).fd == 2); tmp_xml_fd = 2; @@ -897,13 +861,12 @@ void main_process_cmd_line_options ( /*OUT*/Bool* logging_to_fd, but that is likely to confuse the hell out of users, which is distinctly Ungood. */ if (VG_(clo_xml) && tmp_xml_fd == -1) { - VG_(umsg)( + VG_(fmsg_bad_option)( + "--xml=yes, but no XML destination specified", "--xml=yes has been specified, but there is no XML output\n" "destination. You must specify an XML output destination\n" - "using --xml-fd=, --xml-file= or --xml=socket=.\n" ); - /* FIXME: this is really a misuse of VG_(err_bad_option). */ - VG_(err_bad_option)( - "--xml=yes, but no XML destination specified"); + "using --xml-fd, --xml-file or --xml-socket.\n" + ); } // Finalise the output fds: the log fd .. @@ -1133,8 +1096,8 @@ static void print_preamble ( Bool logging_to_fd, VG_(printf_xml_no_f_c)(" %t\n", VG_(name_of_launcher)); else - VG_(printf_xml_no_f_c)(Vg_UserMsg, " %t\n", - "(launcher name unknown)"); + VG_(printf_xml_no_f_c)(" %t\n", + "(launcher name unknown)"); for (i = 0; i < VG_(sizeXA)( VG_(args_for_valgrind) ); i++) { VG_(printf_xml_no_f_c)( " %t\n", diff --git a/coregrind/m_options.c b/coregrind/m_options.c index d6aea74f09..90672c1c33 100644 --- a/coregrind/m_options.c +++ b/coregrind/m_options.c @@ -1,7 +1,6 @@ /*--------------------------------------------------------------------*/ -/*--- Command line options. ---*/ -/*--- m_options.c ---*/ +/*--- Command line options. m_options.c ---*/ /*--------------------------------------------------------------------*/ /* @@ -100,42 +99,9 @@ Bool VG_(clo_dsymutil) = False; /*====================================================================*/ -/*=== Command line errors ===*/ +/*=== File expansion ===*/ /*====================================================================*/ -static void revert_to_stderr ( void ) -{ - VG_(log_output_sink).fd = 2; /* stderr */ - VG_(log_output_sink).is_socket = False; -} - -__attribute__((noreturn)) -void VG_(err_bad_option) ( Char* opt ) -{ - revert_to_stderr(); - VG_(printf)("valgrind: Bad option '%s'; aborting.\n", opt); - VG_(printf)("valgrind: Use --help for more information.\n"); - VG_(exit)(1); -} - -__attribute__((noreturn)) -void VG_(err_missing_prog) ( void ) -{ - revert_to_stderr(); - VG_(printf)("valgrind: no program specified\n"); - VG_(printf)("valgrind: Use --help for more information.\n"); - VG_(exit)(1); -} - -__attribute__((noreturn)) -void VG_(err_config_error) ( Char* msg ) -{ - revert_to_stderr(); - VG_(printf)("valgrind: Startup or configuration error:\n %s\n", msg); - VG_(printf)("valgrind: Unable to start up properly. Giving up.\n"); - VG_(exit)(1); -} - // Copies the string, prepending it with the startup working directory, and // expanding %p and %q entries. Returns a new, malloc'd string. Char* VG_(expand_file_name)(Char* option_name, Char* format) @@ -149,7 +115,7 @@ Char* VG_(expand_file_name)(Char* option_name, Char* format) if (VG_STREQ(format, "")) { // Empty name, bad. - VG_(umsg)("%s: filename is empty", option_name); + VG_(fmsg)("%s: filename is empty", option_name); goto bad; } @@ -158,11 +124,13 @@ Char* VG_(expand_file_name)(Char* option_name, Char* format) // that we don't allow a legitimate filename beginning with '~' but that // seems very unlikely. if (format[0] == '~') { - VG_(umsg)("%s: filename begins with '~'\n", option_name); - VG_(umsg)("You probably expected the shell to expand the '~', but it\n"); - VG_(umsg)("didn't. The rules for '~'-expansion " - "vary from shell to shell.\n"); - VG_(umsg)("You might have more luck using $HOME instead.\n"); + VG_(fmsg)( + "%s: filename begins with '~'\n" + "You probably expected the shell to expand the '~', but it\n" + "didn't. The rules for '~'-expansion vary from shell to shell.\n" + "You might have more luck using $HOME instead.\n", + option_name + ); goto bad; } @@ -216,8 +184,7 @@ Char* VG_(expand_file_name)(Char* option_name, Char* format) qualname = &format[i]; while (True) { if (0 == format[i]) { - VG_(message)(Vg_UserMsg, "%s: malformed %%q specifier\n", - option_name); + VG_(fmsg)("%s: malformed %%q specifier\n", option_name); goto bad; } else if ('}' == format[i]) { // Temporarily replace the '}' with NUL to extract var @@ -225,9 +192,8 @@ Char* VG_(expand_file_name)(Char* option_name, Char* format) format[i] = 0; qual = VG_(getenv)(qualname); if (NULL == qual) { - VG_(message)(Vg_UserMsg, - "%s: environment variable %s is not set\n", - option_name, qualname); + VG_(fmsg)("%s: environment variable %s is not set\n", + option_name, qualname); format[i] = '}'; // Put the '}' back. goto bad; } @@ -240,15 +206,14 @@ Char* VG_(expand_file_name)(Char* option_name, Char* format) ENSURE_THIS_MUCH_SPACE(VG_(strlen)(qual)); j += VG_(sprintf)(&out[j], "%s", qual); } else { - VG_(message)(Vg_UserMsg, - "%s: expected '{' after '%%q'\n", option_name); + VG_(fmsg)("%s: expected '{' after '%%q'\n", option_name); goto bad; } } else { // Something else, abort. - VG_(message)(Vg_UserMsg, - "%s: expected 'p' or 'q' or '%%' after '%%'\n", option_name); + VG_(fmsg)("%s: expected 'p' or 'q' or '%%' after '%%'\n", + option_name); goto bad; } } @@ -265,7 +230,7 @@ Char* VG_(expand_file_name)(Char* option_name, Char* format) VG_(strcpy)(opt, option_name); VG_(strcat)(opt, "="); VG_(strcat)(opt, format); - VG_(err_bad_option)(opt); + VG_(fmsg_bad_option)(opt, ""); } } @@ -335,5 +300,5 @@ Bool VG_(should_we_trace_this_child) ( HChar* child_exe_name ) /*--------------------------------------------------------------------*/ -/*--- end m_options.c ---*/ +/*--- end ---*/ /*--------------------------------------------------------------------*/ diff --git a/coregrind/m_replacemalloc/replacemalloc_core.c b/coregrind/m_replacemalloc/replacemalloc_core.c index 7bcbaaefdb..c861fde4df 100644 --- a/coregrind/m_replacemalloc/replacemalloc_core.c +++ b/coregrind/m_replacemalloc/replacemalloc_core.c @@ -58,12 +58,9 @@ Bool VG_(replacement_malloc_process_cmd_line_option)(Char* arg) VG_(clo_alignment) > 4096 || VG_(log2)( VG_(clo_alignment) ) == -1 /* not a power of 2 */) { - VG_(message)(Vg_UserMsg, - "Invalid --alignment= setting. " - "Should be a power of 2, >= %d, <= 4096.\n", - VG_MIN_MALLOC_SZB - ); - VG_(err_bad_option)("--alignment"); + VG_(fmsg_bad_option)(arg, + "Alignment must be a power of 2 in the range %d..4096.\n", + VG_MIN_MALLOC_SZB); } } diff --git a/coregrind/m_ume/main.c b/coregrind/m_ume/main.c index b0edcf2b9a..947871540b 100644 --- a/coregrind/m_ume/main.c +++ b/coregrind/m_ume/main.c @@ -189,7 +189,7 @@ static Bool is_binary_file(Char* f) // Something went wrong. This will only happen if we earlier // succeeded in opening the file but fail here (eg. the file was // deleted between then and now). - VG_(printf)("valgrind: %s: unknown error\n", f); + VG_(fmsg)("%s: unknown error\n", f); VG_(exit)(126); // 126 == NOEXEC } } @@ -210,7 +210,7 @@ static Int do_exec_shell_followup(Int ret, HChar* exe_name, ExeInfo* info) // Is it a binary file? if (is_binary_file(exe_name)) { - VG_(printf)("valgrind: %s: cannot execute binary file\n", exe_name); + VG_(fmsg)("%s: cannot execute binary file\n", exe_name); VG_(exit)(126); // 126 == NOEXEC } @@ -226,7 +226,7 @@ static Int do_exec_shell_followup(Int ret, HChar* exe_name, ExeInfo* info) if (0 != ret) { // Something went wrong with executing the default interpreter - VG_(printf)("valgrind: %s: bad interpreter (%s): %s\n", + VG_(fmsg)("%s: bad interpreter (%s): %s\n", exe_name, info->interp_name, VG_(strerror)(ret)); VG_(exit)(126); // 126 == NOEXEC } @@ -238,21 +238,20 @@ static Int do_exec_shell_followup(Int ret, HChar* exe_name, ExeInfo* info) // Was it a directory? res = VG_(stat)(exe_name, &st); if (!sr_isError(res) && VKI_S_ISDIR(st.mode)) { - VG_(printf)("valgrind: %s: is a directory\n", exe_name); + VG_(fmsg)("%s: is a directory\n", exe_name); // Was it not executable? } else if (0 != VG_(check_executable)(NULL, exe_name, False/*allow_setuid*/)) { - VG_(printf)("valgrind: %s: %s\n", exe_name, VG_(strerror)(ret)); + VG_(fmsg)("%s: %s\n", exe_name, VG_(strerror)(ret)); // Did it start with "#!"? If so, it must have been a bad interpreter. } else if (is_hash_bang_file(exe_name)) { - VG_(printf)("valgrind: %s: bad interpreter: %s\n", - exe_name, VG_(strerror)(ret)); + VG_(fmsg)("%s: bad interpreter: %s\n", exe_name, VG_(strerror)(ret)); // Otherwise it was something else. } else { - VG_(printf)("valgrind: %s: %s\n", exe_name, VG_(strerror)(ret)); + VG_(fmsg)("%s: %s\n", exe_name, VG_(strerror)(ret)); } // 126 means NOEXEC; I think this is Posix, and that in some cases we // should be returning 127, meaning NOTFOUND. Oh well. diff --git a/coregrind/pub_core_libcprint.h b/coregrind/pub_core_libcprint.h index 9148235c25..5692972c1b 100644 --- a/coregrind/pub_core_libcprint.h +++ b/coregrind/pub_core_libcprint.h @@ -54,6 +54,16 @@ extern OutputSink VG_(xml_output_sink); m_main during startup. */ void VG_(elapsed_wallclock_time) ( /*OUT*/HChar* buf ); +/* Call this if the executable is missing. This function prints an + error message, then shuts down the entire system. */ +__attribute__((noreturn)) +extern void VG_(err_missing_prog) ( void ); + +/* Similarly - complain and stop if there is some kind of config + error. */ +__attribute__((noreturn)) +extern void VG_(err_config_error) ( Char* msg ); + #endif // __PUB_CORE_LIBCPRINT_H /*--------------------------------------------------------------------*/ diff --git a/coregrind/pub_core_options.h b/coregrind/pub_core_options.h index 716a124c06..24125bc89c 100644 --- a/coregrind/pub_core_options.h +++ b/coregrind/pub_core_options.h @@ -210,18 +210,6 @@ extern HChar* VG_(clo_kernel_variant); .dSYM directories as necessary? */ extern Bool VG_(clo_dsymutil); -/* --------- Functions --------- */ - -/* Call this if the executable is missing. This function prints an - error message, then shuts down the entire system. */ -__attribute__((noreturn)) -extern void VG_(err_missing_prog) ( void ); - -/* Similarly - complain and stop if there is some kind of config - error. */ -__attribute__((noreturn)) -extern void VG_(err_config_error) ( Char* msg ); - /* Should we trace into this child executable (across execve etc) ? This involves considering --trace-children=, --trace-children-skip= and the name of the executable. */ diff --git a/include/pub_tool_libcprint.h b/include/pub_tool_libcprint.h index c205515950..c996626505 100644 --- a/include/pub_tool_libcprint.h +++ b/include/pub_tool_libcprint.h @@ -32,19 +32,9 @@ #define __PUB_TOOL_LIBCPRINT_H /* --------------------------------------------------------------------- - Basic printing + Formatting functions ------------------------------------------------------------------ */ -/* Note that they all output to the file descriptor given by the - --log-fd/--log-file/--log-socket argument, which defaults to 2 - (stderr). Hence no need for VG_(fprintf)(). -*/ -extern UInt VG_(printf) ( const HChar *format, ... ) - PRINTF_CHECK(1, 2); - -extern UInt VG_(vprintf) ( const HChar *format, va_list vargs ) - PRINTF_CHECK(1, 0); - extern UInt VG_(sprintf) ( Char* buf, const HChar* format, ... ) PRINTF_CHECK(2, 3); @@ -59,64 +49,97 @@ extern UInt VG_(vsnprintf)( Char* buf, Int size, const HChar *format, va_list vargs ) PRINTF_CHECK(3, 0); -/* Yet another, totally general, version of vprintf, which hands all - output bytes to CHAR_SINK, passing it OPAQUE as the second arg. */ -extern void VG_(vcbprintf)( void(*char_sink)(HChar, void* opaque), - void* opaque, - const HChar* format, va_list vargs ); - -/* These are the same as the non "_xml" versions above, except the - output goes on the selected XML output channel instead of the - normal one. -*/ -extern UInt VG_(printf_xml) ( const HChar *format, ... ) - PRINTF_CHECK(1, 2); - -extern UInt VG_(vprintf_xml) ( const HChar *format, va_list vargs ) - PRINTF_CHECK(1, 0); - -extern UInt VG_(printf_xml_no_f_c) ( const HChar *format, ... ); - // Percentify n/m with d decimal places. Includes the '%' symbol at the end. // Right justifies in 'buf'. extern void VG_(percentify)(ULong n, ULong m, UInt d, Int n_buf, char buf[]); /* --------------------------------------------------------------------- - Messages for the user + Output-printing functions ------------------------------------------------------------------ */ +// Note that almost all output goes to the file descriptor given by the +// --log-fd/--log-file/--log-socket argument, which defaults to 2 (stderr). +// (Except that some text always goes to stdout/stderr at startup, and +// debugging messages always go to stderr.) Hence no need for +// VG_(fprintf)(). + /* No, really. I _am_ that strange. */ #define OINK(nnn) VG_(message)(Vg_DebugMsg, "OINK %d\n",nnn) -/* Print a message prefixed by "???? "; '?' depends on the VgMsgKind. +/* Print a message with a prefix that depends on the VgMsgKind. Should be used for all user output. */ typedef - enum { Vg_UserMsg, /* '?' == '=' */ - Vg_DebugMsg, /* '?' == '-' */ - Vg_DebugExtraMsg, /* '?' == '+' */ - Vg_ClientMsg /* '?' == '*' */ + enum { // Prefix + Vg_FailMsg, // "valgrind:" + Vg_UserMsg, // "==pid==" + Vg_DebugMsg, // "--pid--" + Vg_DebugExtraMsg, // "++pid++" + Vg_ClientMsg // "**pid**" } VgMsgKind; -/* Send a single-part message. The format specification may contain - any ISO C format specifier or %t. No attempt is made to let the - compiler verify consistency of the format string and the argument - list. */ +// These print output that isn't prefixed with anything, and should be +// used in very few cases, such as printing usage messages. +extern UInt VG_(printf) ( const HChar *format, ... ) + PRINTF_CHECK(1, 2); +extern UInt VG_(vprintf) ( const HChar *format, va_list vargs ) + PRINTF_CHECK(1, 0); + +// The "_no_f_c" functions here are just like their non-"_no_f_c" counterparts +// but without the PRINTF_CHECK, so they can be used with our non-standard %t +// format specifier. + +// These are the same as the non "_xml" versions above, except the +// output goes on the selected XML output channel instead of the +// normal one. +extern UInt VG_(printf_xml) ( const HChar *format, ... ) + PRINTF_CHECK(1, 2); + +extern UInt VG_(vprintf_xml) ( const HChar *format, va_list vargs ) + PRINTF_CHECK(1, 0); + +extern UInt VG_(printf_xml_no_f_c) ( const HChar *format, ... ); + +/* Yet another, totally general, version of vprintf, which hands all + output bytes to CHAR_SINK, passing it OPAQUE as the second arg. */ +extern void VG_(vcbprintf)( void(*char_sink)(HChar, void* opaque), + void* opaque, + const HChar* format, va_list vargs ); + extern UInt VG_(message_no_f_c)( VgMsgKind kind, const HChar* format, ... ); -/* Send a single-part message. The format specification may contain - any ISO C format specifier. The gcc compiler will verify - consistency of the format string and the argument list. */ extern UInt VG_(message)( VgMsgKind kind, const HChar* format, ... ) - PRINTF_CHECK(2, 3); + PRINTF_CHECK(2, 3); extern UInt VG_(vmessage)( VgMsgKind kind, const HChar* format, va_list vargs ) - PRINTF_CHECK(2, 0); + PRINTF_CHECK(2, 0); // Short-cuts for VG_(message)(). + +// This is used for messages printed due to start-up failures that occur +// before the preamble is printed, eg. due a bad executable. +extern UInt VG_(fmsg)( const HChar* format, ... ) PRINTF_CHECK(1, 2); + +// This is used if an option was bad for some reason. Note: don't use it just +// because an option was unrecognised -- return 'False' from +// VG_(tdict).tool_process_cmd_line_option) to indicate that -- use it if eg. +// an option was given an inappropriate argument. This function prints an +// error message, then shuts down the entire system. +__attribute__((noreturn)) +extern void VG_(fmsg_bad_option) ( HChar* opt, const HChar* format, ... ) + PRINTF_CHECK(2, 3); + +// This is used for messages that are interesting to the user: info about +// their program (eg. preamble, tool error messages, postamble) or stuff they +// requested. extern UInt VG_(umsg)( const HChar* format, ... ) PRINTF_CHECK(1, 2); + +// This is used for debugging messages that are only of use to developers. extern UInt VG_(dmsg)( const HChar* format, ... ) PRINTF_CHECK(1, 2); + +// This is used for additional debugging messages that are only of use to +// developers. extern UInt VG_(emsg)( const HChar* format, ... ) PRINTF_CHECK(1, 2); /* Flush any output cached by previous calls to VG_(message) et al. */ diff --git a/include/pub_tool_options.h b/include/pub_tool_options.h index f28cc70970..7f85492518 100644 --- a/include/pub_tool_options.h +++ b/include/pub_tool_options.h @@ -77,7 +77,7 @@ Long n = VG_(strtoll10)( val, &s ); \ (qq_var) = n; \ /* Check for non-numeralness, or overflow. */ \ - if ('\0' != s[0] || (qq_var) != n) VG_(err_bad_option)(qq_arg); \ + if ('\0' != s[0] || (qq_var) != n) VG_(fmsg_bad_option)(qq_arg, ""); \ True; \ }) \ ) @@ -91,15 +91,16 @@ Char* s; \ Long n = VG_(strtoll##qq_base)( val, &s ); \ (qq_var) = n; \ + /* MMM: separate the two cases, and explain the problem; likewise */ \ + /* for all the other macros in this file. */ \ /* Check for non-numeralness, or overflow. */ \ /* Nb: it will overflow if qq_var is unsigned and qq_val is negative! */ \ - if ('\0' != s[0] || (qq_var) != n) VG_(err_bad_option)(qq_arg); \ + if ('\0' != s[0] || (qq_var) != n) VG_(fmsg_bad_option)(qq_arg, ""); \ /* Check bounds. */ \ if ((qq_var) < (qq_lo) || (qq_var) > (qq_hi)) { \ - VG_(message)(Vg_UserMsg, \ - "'%s' argument must be between %lld and %lld\n", \ - (qq_option), (Long)(qq_lo), (Long)(qq_hi)); \ - VG_(err_bad_option)(qq_arg); \ + VG_(fmsg_bad_option)(qq_arg, \ + "'%s' argument must be between %lld and %lld\n", \ + (qq_option), (Long)(qq_lo), (Long)(qq_hi)); \ } \ True; \ }) \ @@ -124,7 +125,7 @@ double n = VG_(strtod)( val, &s ); \ (qq_var) = n; \ /* Check for non-numeralness */ \ - if ('\0' != s[0]) VG_(err_bad_option)(qq_arg); \ + if ('\0' != s[0]) VG_(fmsg_bad_option)(qq_arg, ""); \ True; \ }) \ ) @@ -165,15 +166,6 @@ extern Int VG_(clo_backtrace_size); extern Bool VG_(clo_show_below_main); -/* Call this if a recognised option was bad for some reason. Note: - don't use it just because an option was unrecognised -- return - 'False' from VG_(tdict).tool_process_cmd_line_option) to indicate that -- - use it if eg. an option was given an inappropriate argument. - This function prints an error message, then shuts down the entire system. - It returns a Bool so it can be used in the _CLO_ macros. */ -__attribute__((noreturn)) -extern void VG_(err_bad_option) ( Char* opt ); - /* Used to expand file names. "option_name" is the option name, eg. "--log-file". 'format' is what follows, eg. "cachegrind.out.%p". In 'format': diff --git a/include/pub_tool_tooliface.h b/include/pub_tool_tooliface.h index bb39097cf1..ac346b9ec2 100644 --- a/include/pub_tool_tooliface.h +++ b/include/pub_tool_tooliface.h @@ -374,10 +374,17 @@ extern void VG_(needs_superblock_discards) ( /* Tool defines its own command line options? */ extern void VG_(needs_command_line_options) ( - // Return True if option was recognised. Presumably sets some state to - // record the option as well. Nb: tools can assume that the argv will - // never disappear. So they can, for example, store a pointer to a string - // within an option, rather than having to make a copy. + // Return True if option was recognised, False if it wasn't (but also see + // below). Presumably sets some state to record the option as well. + // + // Nb: tools can assume that the argv will never disappear. So they can, + // for example, store a pointer to a string within an option, rather than + // having to make a copy. + // + // Options (and combinations of options) should be checked in this function + // if possible rather than in post_clo_init(), and if they are bad then + // VG_(fmsg_bad_option)() should be called. This ensures that the + // messaging is consistent with command line option errors from the core. Bool (*process_cmd_line_option)(Char* argv), // Print out command line usage for options for normal tool operation. diff --git a/massif/ms_main.c b/massif/ms_main.c index 0b88bc37e5..c34d27b805 100644 --- a/massif/ms_main.c +++ b/massif/ms_main.c @@ -442,7 +442,12 @@ static Bool ms_process_cmd_line_option(Char* arg) VG_(addToXA)(ignore_fns, &tmp_str); } - else if VG_DBL_CLO(arg, "--threshold", clo_threshold) {} + else if VG_DBL_CLO(arg, "--threshold", clo_threshold) { + if (clo_threshold < 0 || clo_threshold > 100) { + VG_(fmsg_bad_option)(arg, + "--threshold must be between 0.0 and 100.0\n"); + } + } else if VG_DBL_CLO(arg, "--peak-inaccuracy", clo_peak_inaccuracy) {} @@ -2386,14 +2391,10 @@ static void ms_post_clo_init(void) Char* s2; // Check options. - if (clo_threshold < 0 || clo_threshold > 100) { - VG_(umsg)("--threshold must be between 0.0 and 100.0\n"); - VG_(err_bad_option)("--threshold"); - } if (clo_pages_as_heap) { if (clo_stacks) { - VG_(umsg)("--pages-as-heap=yes cannot be used with --stacks=yes\n"); - VG_(err_bad_option)("--pages-as-heap=yes with --stacks=yes"); + VG_(fmsg_bad_option)( + "--pages-as-heap=yes together with --stacks=yes", ""); } } if (!clo_heap) { diff --git a/memcheck/mc_main.c b/memcheck/mc_main.c index 2ddf71fdf0..1e4d843d44 100644 --- a/memcheck/mc_main.c +++ b/memcheck/mc_main.c @@ -4752,8 +4752,6 @@ Int MC_(clo_mc_level) = 2; static Bool mc_process_cmd_line_options(Char* arg) { Char* tmp_str; - Char* bad_level_msg = - "ERROR: --track-origins=yes has no effect when --undef-value-errors=no"; tl_assert( MC_(clo_mc_level) >= 1 && MC_(clo_mc_level) <= 3 ); @@ -4768,8 +4766,7 @@ static Bool mc_process_cmd_line_options(Char* arg) */ if (0 == VG_(strcmp)(arg, "--undef-value-errors=no")) { if (MC_(clo_mc_level) == 3) { - VG_(message)(Vg_DebugMsg, "%s\n", bad_level_msg); - return False; + goto bad_level; } else { MC_(clo_mc_level) = 1; return True; @@ -4787,8 +4784,7 @@ static Bool mc_process_cmd_line_options(Char* arg) } if (0 == VG_(strcmp)(arg, "--track-origins=yes")) { if (MC_(clo_mc_level) == 1) { - VG_(message)(Vg_DebugMsg, "%s\n", bad_level_msg); - return False; + goto bad_level; } else { MC_(clo_mc_level) = 3; return True; @@ -4854,6 +4850,11 @@ static Bool mc_process_cmd_line_options(Char* arg) return VG_(replacement_malloc_process_cmd_line_option)(arg); return True; + + + bad_level: + VG_(fmsg_bad_option)(arg, + "--track-origins=yes has no effect when --undef-value-errors=no.\n"); } static void mc_print_usage(void) diff --git a/none/tests/cmdline2.stdout.exp b/none/tests/cmdline2.stdout.exp index 60e0328f28..caa7eb0689 100644 --- a/none/tests/cmdline2.stdout.exp +++ b/none/tests/cmdline2.stdout.exp @@ -65,8 +65,8 @@ usage: valgrind [options] prog-and-args (none) debugging options for all Valgrind tools: - --stats=no|yes show tool and core statistics [no] -d show verbose debugging output + --stats=no|yes show tool and core statistics [no] --sanity-level= level of sanity checking to do [1] --trace-flags= show generated code? (X = 0|1) [00000000] --profile-flags= ditto, but for profiling (X = 0|1) [00000000] diff --git a/none/tests/cmdline4.stderr.exp b/none/tests/cmdline4.stderr.exp index 1f88235fdc..f9b9ba6585 100644 --- a/none/tests/cmdline4.stderr.exp +++ b/none/tests/cmdline4.stderr.exp @@ -1,2 +1,2 @@ -valgrind: Bad option '--bad-bad-option'; aborting. -valgrind: Use --help for more information. +valgrind: Bad option: --bad-bad-option +valgrind: Use --help for more information or consult the user manual. -- 2.47.2