From: Paul Floyd Date: Sat, 12 Nov 2022 18:24:51 +0000 (+0100) Subject: Bug 351857 - confusing error message about valid command line option X-Git-Tag: VALGRIND_3_21_0~275 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=ac6d9faf4b7014d9eb2b7f2a21044460eb67a94b;p=thirdparty%2Fvalgrind.git Bug 351857 - confusing error message about valid command line option Added code to handle missing "=something". --- diff --git a/NEWS b/NEWS index a421b980c5..b8bed8ff0c 100644 --- a/NEWS +++ b/NEWS @@ -21,6 +21,7 @@ than mailing the developers (or mailing lists) directly -- bugs that are not entered into bugzilla tend to get forgotten about or ignored. 170510 Don't warn about ioctl of size 0 without direction hint +351857 confusing error message about valid command line option 444110 priv/guest_ppc_toIR.c:36198:31: warning: duplicated 'if' condition. 459476 vgdb: allow address reuse to avoid "address already in use" errorsuse" errors diff --git a/drd/drd_main.c b/drd/drd_main.c index 4a71eebb5f..2161e0316e 100644 --- a/drd/drd_main.c +++ b/drd/drd_main.c @@ -65,60 +65,95 @@ static Bool trace_sectsuppr; */ static Bool DRD_(process_cmd_line_option)(const HChar* arg) { - int check_stack_accesses = -1; - int join_list_vol = -1; - int exclusive_threshold_ms = -1; - int first_race_only = -1; - int report_signal_unlocked = -1; - int segment_merging = -1; - int segment_merge_interval = -1; - int shared_threshold_ms = -1; - int show_confl_seg = -1; - int trace_barrier = -1; - int trace_clientobj = -1; - int trace_cond = -1; - int trace_csw = -1; - int trace_fork_join = -1; - int trace_hb = -1; - int trace_conflict_set = -1; - int trace_conflict_set_bm = -1; - int trace_mutex = -1; - int trace_rwlock = -1; - int trace_segment = -1; - int trace_semaphore = -1; - int trace_suppression = -1; + Bool check_stack_accesses = False; + int join_list_vol = -1; + int exclusive_threshold_ms = -1; + Bool first_race_only = False; + Bool report_signal_unlocked = False; + Bool segment_merging = False; + int segment_merge_interval = -1; + int shared_threshold_ms = -1; + Bool show_confl_seg = False; + Bool trace_barrier = False; + Bool trace_clientobj = False; + Bool trace_cond = False; + Bool trace_csw = False; + Bool trace_fork_join = False; + Bool trace_hb = False; + Bool trace_conflict_set = False; + Bool trace_conflict_set_bm = False; + Bool trace_mutex = False; + Bool trace_rwlock = False; + Bool trace_segment = False; + Bool trace_semaphore = False; + Bool trace_suppression = False; const HChar* trace_address = 0; const HChar* ptrace_address= 0; - if VG_BOOL_CLO(arg, "--check-stack-var", check_stack_accesses) {} + if VG_BOOL_CLO(arg, "--check-stack-var", check_stack_accesses) { + DRD_(set_check_stack_accesses)(check_stack_accesses); + } else if VG_INT_CLO (arg, "--join-list-vol", join_list_vol) {} else if VG_BOOL_CLO(arg, "--drd-stats", s_print_stats) {} - else if VG_BOOL_CLO(arg, "--first-race-only", first_race_only) {} + else if VG_BOOL_CLO(arg, "--first-race-only", first_race_only) { + DRD_(set_first_race_only)(first_race_only); + } else if VG_BOOL_CLO(arg, "--free-is-write", DRD_(g_free_is_write)) {} - else if VG_BOOL_CLO(arg,"--report-signal-unlocked",report_signal_unlocked) - {} - else if VG_BOOL_CLO(arg, "--segment-merging", segment_merging) {} + else if VG_BOOL_CLO(arg,"--report-signal-unlocked",report_signal_unlocked) { + DRD_(cond_set_report_signal_unlocked)(report_signal_unlocked); + } + else if VG_BOOL_CLO(arg, "--segment-merging", segment_merging) { + DRD_(thread_set_segment_merging)(segment_merging); + } else if VG_INT_CLO (arg, "--segment-merging-interval", segment_merge_interval) {} - else if VG_BOOL_CLO(arg, "--show-confl-seg", show_confl_seg) {} + else if VG_BOOL_CLO(arg, "--show-confl-seg", show_confl_seg) { + DRD_(set_show_conflicting_segments)(show_confl_seg); + } else if VG_BOOL_CLO(arg, "--show-stack-usage", s_show_stack_usage) {} else if VG_BOOL_CLO(arg, "--ignore-thread-creation", DRD_(ignore_thread_creation)) {} else if VG_BOOL_CLO(arg, "--trace-alloc", s_trace_alloc) {} - else if VG_BOOL_CLO(arg, "--trace-barrier", trace_barrier) {} - else if VG_BOOL_CLO(arg, "--trace-clientobj", trace_clientobj) {} - else if VG_BOOL_CLO(arg, "--trace-cond", trace_cond) {} - else if VG_BOOL_CLO(arg, "--trace-conflict-set", trace_conflict_set) {} - else if VG_BOOL_CLO(arg, "--trace-conflict-set-bm", trace_conflict_set_bm){} - else if VG_BOOL_CLO(arg, "--trace-csw", trace_csw) {} - else if VG_BOOL_CLO(arg, "--trace-fork-join", trace_fork_join) {} - else if VG_BOOL_CLO(arg, "--trace-hb", trace_hb) {} - else if VG_BOOL_CLO(arg, "--trace-mutex", trace_mutex) {} - else if VG_BOOL_CLO(arg, "--trace-rwlock", trace_rwlock) {} + else if VG_BOOL_CLO(arg, "--trace-barrier", trace_barrier) { + DRD_(barrier_set_trace)(trace_barrier); + } + else if VG_BOOL_CLO(arg, "--trace-clientobj", trace_clientobj) { + DRD_(clientobj_set_trace)(trace_clientobj); + } + else if VG_BOOL_CLO(arg, "--trace-cond", trace_cond) { + DRD_(cond_set_trace)(trace_cond); + } + else if VG_BOOL_CLO(arg, "--trace-conflict-set", trace_conflict_set) { + DRD_(thread_trace_conflict_set)(trace_conflict_set); + } + else if VG_BOOL_CLO(arg, "--trace-conflict-set-bm", trace_conflict_set_bm){ + DRD_(thread_trace_conflict_set_bm)(trace_conflict_set_bm); + } + else if VG_BOOL_CLO(arg, "--trace-csw", trace_csw) { + DRD_(thread_trace_context_switches)(trace_csw); + } + else if VG_BOOL_CLO(arg, "--trace-fork-join", trace_fork_join) { + DRD_(thread_set_trace_fork_join)(trace_fork_join); + } + else if VG_BOOL_CLO(arg, "--trace-hb", trace_hb) { + DRD_(hb_set_trace)(trace_hb); + } + else if VG_BOOL_CLO(arg, "--trace-mutex", trace_mutex) { + DRD_(mutex_set_trace)(trace_mutex); + } + else if VG_BOOL_CLO(arg, "--trace-rwlock", trace_rwlock) { + DRD_(rwlock_set_trace)(trace_rwlock); + } else if VG_BOOL_CLO(arg, "--trace-sectsuppr", trace_sectsuppr) {} - else if VG_BOOL_CLO(arg, "--trace-segment", trace_segment) {} - else if VG_BOOL_CLO(arg, "--trace-semaphore", trace_semaphore) {} - else if VG_BOOL_CLO(arg, "--trace-suppr", trace_suppression) {} + else if VG_BOOL_CLO(arg, "--trace-segment", trace_segment) { + DRD_(sg_set_trace)(trace_segment); + } + else if VG_BOOL_CLO(arg, "--trace-semaphore", trace_semaphore) { + DRD_(semaphore_set_trace)(trace_semaphore); + } + else if VG_BOOL_CLO(arg, "--trace-suppr", trace_suppression) { + DRD_(suppression_set_trace)(trace_suppression); + } else if VG_BOOL_CLO(arg, "--var-info", s_var_info) {} else if VG_BOOL_CLO(arg, "--verify-conflict-set", DRD_(verify_conflict_set)) {} @@ -129,33 +164,19 @@ static Bool DRD_(process_cmd_line_option)(const HChar* arg) else return VG_(replacement_malloc_process_cmd_line_option)(arg); - if (check_stack_accesses != -1) - DRD_(set_check_stack_accesses)(check_stack_accesses); if (exclusive_threshold_ms != -1) { DRD_(mutex_set_lock_threshold)(exclusive_threshold_ms); DRD_(rwlock_set_exclusive_threshold)(exclusive_threshold_ms); } - if (first_race_only != -1) - { - DRD_(set_first_race_only)(first_race_only); - } if (join_list_vol != -1) DRD_(thread_set_join_list_vol)(join_list_vol); - if (report_signal_unlocked != -1) - { - DRD_(cond_set_report_signal_unlocked)(report_signal_unlocked); - } if (shared_threshold_ms != -1) { DRD_(rwlock_set_shared_threshold)(shared_threshold_ms); } - if (segment_merging != -1) - DRD_(thread_set_segment_merging)(segment_merging); if (segment_merge_interval != -1) DRD_(thread_set_segment_merge_interval)(segment_merge_interval); - if (show_confl_seg != -1) - DRD_(set_show_conflicting_segments)(show_confl_seg); if (trace_address) { const Addr addr = VG_(strtoll16)(trace_address, 0); DRD_(start_tracing_address_range)(addr, addr + 1, False); @@ -169,32 +190,6 @@ static Bool DRD_(process_cmd_line_option)(const HChar* arg) length = plus ? VG_(strtoll16)(plus + 1, 0) : 1; DRD_(start_tracing_address_range)(addr, addr + length, True); } - if (trace_barrier != -1) - DRD_(barrier_set_trace)(trace_barrier); - if (trace_clientobj != -1) - DRD_(clientobj_set_trace)(trace_clientobj); - if (trace_cond != -1) - DRD_(cond_set_trace)(trace_cond); - if (trace_csw != -1) - DRD_(thread_trace_context_switches)(trace_csw); - if (trace_fork_join != -1) - DRD_(thread_set_trace_fork_join)(trace_fork_join); - if (trace_hb != -1) - DRD_(hb_set_trace)(trace_hb); - if (trace_conflict_set != -1) - DRD_(thread_trace_conflict_set)(trace_conflict_set); - if (trace_conflict_set_bm != -1) - DRD_(thread_trace_conflict_set_bm)(trace_conflict_set_bm); - if (trace_mutex != -1) - DRD_(mutex_set_trace)(trace_mutex); - if (trace_rwlock != -1) - DRD_(rwlock_set_trace)(trace_rwlock); - if (trace_segment != -1) - DRD_(sg_set_trace)(trace_segment); - if (trace_semaphore != -1) - DRD_(semaphore_set_trace)(trace_semaphore); - if (trace_suppression != -1) - DRD_(suppression_set_trace)(trace_suppression); return True; } diff --git a/include/pub_tool_options.h b/include/pub_tool_options.h index 1879226e80..39c4137349 100644 --- a/include/pub_tool_options.h +++ b/include/pub_tool_options.h @@ -30,6 +30,8 @@ #define __PUB_TOOL_OPTIONS_H #include "pub_tool_basics.h" // for VG_ macro +#include "pub_tool_libcbase.h" // for VG__ str functions +#include "pub_tool_libcprint.h" // for VG_(fmsg_bad_option) #include "libvex.h" // for VexControl // Command line option parsing happens in the following modes: @@ -118,22 +120,47 @@ extern void VG_(list_clo)(const HChar *qq_option); (qq_mode, qq_arg, qq_option, \ VG_STREQ(qq_arg, qq_option))) +static inline +Bool VG_(bool_clom)(Clo_Mode qq_mode, const HChar* qq_arg, const HChar* qq_option, Bool* qq_var, Bool qq_vareq_arg) +{ + Bool res = False; + if (VG_(check_clom)(qq_mode, qq_arg, qq_option, qq_vareq_arg)) + { + const HChar* val = &(qq_arg)[ VG_(strlen)(qq_option)+1 ]; + if (VG_(strcmp)(val, "yes") == 0) + { + *qq_var = True; + res = True; + } + else if (VG_(strcmp)(val, "no") == 0) + { + *qq_var = False; + res = True; + } + else + { + VG_(fmsg_bad_option)(qq_arg, "Invalid boolean value '%s'" + " (should be 'yes' or 'no')\n", + /* gcc 10 (20200119) complains that |val| could be null here. */ + /* I think it is wrong, but anyway, to placate it .. */ + (val ? val : "(null)")); + } + } else if (VG_STREQN(VG_(strlen)(qq_option), qq_arg, qq_option) && + VG_(strlen)(qq_option) == VG_(strlen)(qq_arg)) + { + VG_(fmsg_bad_option)(qq_arg, + "Missing boolean value, did you mean '%s=yes'?\n", + qq_arg); + } + + return res; +} + // String argument, eg. --foo=yes or --foo=no #define VG_BOOL_CLOM(qq_mode, qq_arg, qq_option, qq_var) \ - (VG_(check_clom) \ - (qq_mode, qq_arg, qq_option, \ - VG_STREQN(VG_(strlen)(qq_option)+1, qq_arg, qq_option"=")) && \ - ({Bool res = True; \ - const HChar* val = &(qq_arg)[ VG_(strlen)(qq_option)+1 ]; \ - if VG_STREQ(val, "yes") (qq_var) = True; \ - else if VG_STREQ(val, "no") (qq_var) = False; \ - else {VG_(fmsg_bad_option)(qq_arg, "Invalid boolean value '%s'" \ - " (should be 'yes' or 'no')\n", \ - /* gcc 10 (20200119) complains that |val| could be null here. */ \ - /* I think it is wrong, but anyway, to placate it .. */ \ - (val ? val : "(null)")); \ - res = False; } \ - res; })) + (VG_(bool_clom)((qq_mode), (qq_arg), (qq_option), &(qq_var), \ + VG_STREQN(VG_(strlen)(qq_option)+1, qq_arg, qq_option"=")) \ + ) #define VG_BOOL_CLO(qq_arg, qq_option, qq_var) \ VG_BOOL_CLOM(cloP, qq_arg, qq_option, qq_var) diff --git a/memcheck/mc_main.c b/memcheck/mc_main.c index 979a654097..141cfe19e1 100644 --- a/memcheck/mc_main.c +++ b/memcheck/mc_main.c @@ -6077,7 +6077,7 @@ static const HChar * MC_(parse_leak_heuristics_tokens) = static Bool mc_process_cmd_line_options(const HChar* arg) { const HChar* tmp_str; - Int tmp_show; + Bool tmp_show; tl_assert( MC_(clo_mc_level) >= 1 && MC_(clo_mc_level) <= 3 ); diff --git a/none/tests/Makefile.am b/none/tests/Makefile.am index b3814df638..871ec62777 100644 --- a/none/tests/Makefile.am +++ b/none/tests/Makefile.am @@ -111,6 +111,11 @@ EXTRA_DIST = \ cmdline4.stderr.exp cmdline4.vgtest \ cmdline5.stderr.exp cmdline5.vgtest \ cmdline6.stderr.exp cmdline6.vgtest \ + cmdline7.stderr.exp cmdline7.vgtest \ + cmdline8.stderr.exp cmdline8.vgtest \ + cmdline9.stderr.exp cmdline9.vgtest \ + cmdline10.stderr.exp cmdline10.vgtest \ + cmdline11.stderr.exp cmdline11.vgtest \ cmd-with-special.stderr.exp cmd-with-special.vgtest \ coolo_sigaction.stderr.exp \ coolo_sigaction.stdout.exp coolo_sigaction.vgtest \ diff --git a/none/tests/cmdline10.stderr.exp b/none/tests/cmdline10.stderr.exp new file mode 100644 index 0000000000..38e2917cf2 --- /dev/null +++ b/none/tests/cmdline10.stderr.exp @@ -0,0 +1,3 @@ +valgrind: Bad option: --trace-children=foo +valgrind: Invalid boolean value 'foo' (should be 'yes' or 'no') +valgrind: Use --help for more information or consult the user manual. diff --git a/none/tests/cmdline10.vgtest b/none/tests/cmdline10.vgtest new file mode 100644 index 0000000000..99d04345bd --- /dev/null +++ b/none/tests/cmdline10.vgtest @@ -0,0 +1,3 @@ +# A boolean arg, bad val +prog: ../../tests/true +vgopts: --trace-children=foo diff --git a/none/tests/cmdline11.stderr.exp b/none/tests/cmdline11.stderr.exp new file mode 100644 index 0000000000..ae92ec9ff5 --- /dev/null +++ b/none/tests/cmdline11.stderr.exp @@ -0,0 +1,3 @@ +valgrind: Bad option: --trace-children +valgrind: Missing boolean value, did you mean '--trace-children=yes'? +valgrind: Use --help for more information or consult the user manual. diff --git a/none/tests/cmdline11.vgtest b/none/tests/cmdline11.vgtest new file mode 100644 index 0000000000..2b9e64d8ad --- /dev/null +++ b/none/tests/cmdline11.vgtest @@ -0,0 +1,4 @@ +# A boolean arg no equals or arg +# this one for https://bugs.kde.org/show_bug.cgi?id=351857 +prog: ../../tests/true +vgopts: --trace-children diff --git a/none/tests/cmdline7.stderr.exp b/none/tests/cmdline7.stderr.exp new file mode 100644 index 0000000000..139597f9cb --- /dev/null +++ b/none/tests/cmdline7.stderr.exp @@ -0,0 +1,2 @@ + + diff --git a/none/tests/cmdline7.vgtest b/none/tests/cmdline7.vgtest new file mode 100644 index 0000000000..22691688ed --- /dev/null +++ b/none/tests/cmdline7.vgtest @@ -0,0 +1,3 @@ +# a boolean arg, no +prog: ../../tests/true +vgopts: --trace-children=no diff --git a/none/tests/cmdline8.stderr.exp b/none/tests/cmdline8.stderr.exp new file mode 100644 index 0000000000..139597f9cb --- /dev/null +++ b/none/tests/cmdline8.stderr.exp @@ -0,0 +1,2 @@ + + diff --git a/none/tests/cmdline8.vgtest b/none/tests/cmdline8.vgtest new file mode 100644 index 0000000000..ecbc01fcee --- /dev/null +++ b/none/tests/cmdline8.vgtest @@ -0,0 +1,3 @@ +# a boolean arg, yes +prog: ../../tests/true +vgopts: --trace-children=yes diff --git a/none/tests/cmdline9.stderr.exp b/none/tests/cmdline9.stderr.exp new file mode 100644 index 0000000000..6e965c8dc0 --- /dev/null +++ b/none/tests/cmdline9.stderr.exp @@ -0,0 +1,3 @@ +valgrind: Bad option: --trace-children= +valgrind: Invalid boolean value '' (should be 'yes' or 'no') +valgrind: Use --help for more information or consult the user manual. diff --git a/none/tests/cmdline9.vgtest b/none/tests/cmdline9.vgtest new file mode 100644 index 0000000000..cd4fe56d0b --- /dev/null +++ b/none/tests/cmdline9.vgtest @@ -0,0 +1,3 @@ +# a boolean arg, nothing after = +prog: ../../tests/true +vgopts: --trace-children=