Bug 351857 - confusing error message about valid command line option

Added code to handle missing "=something".

diff --git a/NEWS b/NEWS
index a421b980c5def24ca16a828450763c7eabe6fbdc..b8bed8ff0c0664ec31140eaa11f0b2a5fa3b36e9 100644 (file)
--- 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 4a71eebb5fc816a904d1ba41b0984dda50b16ea1..2161e0316e15a09f51f9e6c1e37e930c6575c327 100644 (file)
--- 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 1879226e800b5382a3f3f38f04f489a00953107c..39c4137349c634b707d10b3271de9c7630d39c1c 100644 (file)
--- 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 979a654097bb61435d372d243369353a3ceb418e..141cfe19e1332f3ce2bc855aa1bd428893dfc8b3 100644 (file)
--- 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 b3814df6380fc5a3e6200246acaf673767108de9..871ec6277739628a5c1407ff948fdd38be77d3ec 100644 (file)
--- 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 (file)
index 0000000..38e2917
--- /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 (file)
index 0000000..99d0434
--- /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 (file)
index 0000000..ae92ec9
--- /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 (file)
index 0000000..2b9e64d
--- /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 (file)
index 0000000..139597f
--- /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 (file)
index 0000000..2269168
--- /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 (file)
index 0000000..139597f
--- /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 (file)
index 0000000..ecbc01f
--- /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 (file)
index 0000000..6e965c8
--- /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 (file)
index 0000000..cd4fe56
--- /dev/null
+++ b/none/tests/cmdline9.vgtest
@@ -0,0 +1,3 @@
+# a boolean arg, nothing after =
+prog: ../../tests/true
+vgopts: --trace-children=