Re: results of gnulib tests with -fcheck-pointer-bounds

[Bruno Haible]

Hi Paul,

> > The message "Saw a #BR!" is a bit cryptic
> > Does someone understand this argp-help.c code?
> 
> I didn't, but after looking at the code for a bit I see a problem that 
> could explain the symptoms you observe. hol_append subtracts pointers 
> into different arrays, which has undefined behavior in C, and 
> -fcheck-pointer-bounds can catch this after the resulting offset is used 
> to calculate a pointer and the pointer is then later used. This is 
> clearly a portability bug so I installed the first attached patch. Does 
> it fix the problem on your platform?

Yes, it fixes the problem. Great! Thanks.

> I also tested argp under -fsanitize=undefined and found a different bug,
> fixed in the 2nd attached patch.

Sorry, I don't understand this patch: it avoids the undefined behaviour
on left-shift, but still makes assumptions about the "implementation-
defined" behaviour of right-shift [1], here:
        user_key = shifted_user_key >> GROUP_BITS;
[1] 
http://stackoverflow.com/questions/7622/are-the-shift-operators-arithmetic-or-logical-in-c

How about this? The intended operation (in terms of bits) is:

      z.......abcdefghijklmnopqrstuvwx
->    zzzzzzzzabcdefghijklmnopqrstuvwx

AFAIK, bit operations (&, |, ~) don't have undefined or implementation-defined
behaviour on signed integer types. Once we assume two's complement - which your
comment already states - they are unambiguous. Proposed patch:


2017-05-20  Bruno Haible  <address@hidden>

        argp: Simplify bit manipulation.
        * lib/argp-parse.c (parser_parse_opt): Use &, |, ~ instead of shifts
        on a signed integer type.

diff --git a/lib/argp-parse.c b/lib/argp-parse.c
index 4723a2b..fb04a4a 100644
--- a/lib/argp-parse.c
+++ b/lib/argp-parse.c
@@ -743,15 +743,8 @@ parser_parse_opt (struct parser *parser, int opt, char 
*val)
     /* A long option.  Preserve the sign in the user key, without
        invoking undefined behavior.  Assume two's complement.  */
     {
-      unsigned uopt = opt;
-      unsigned ushifted_user_key = uopt << GROUP_BITS;
-      int shifted_user_key = ushifted_user_key;
-      int user_key;
-      if (-1 >> 1 == -1)
-        user_key = shifted_user_key >> GROUP_BITS;
-      else
-        user_key = ((ushifted_user_key >> GROUP_BITS)
-                    - (shifted_user_key < 0 ? 1 << USER_BITS : 0));
+      int user_key =
+        ((opt & (1 << (USER_BITS - 1))) ? ~USER_MASK : 0) | (opt & USER_MASK);
       err =
         group_parse (&parser->groups[group_key - 1], &parser->state,
                      user_key, parser->opt_data.optarg);