[PATCH 10/11] argp: Fix shift bug

[Alexander Monakov]

> >> -    /* A long option.  We use shifts instead of masking for extracting
> >> -       the user value in order to preserve the sign.  */
> >> -    err =
> >> -      group_parse (&parser->groups[group_key - 1], &parser->state,
> >> -		   (opt << GROUP_BITS) >> GROUP_BITS,
> >> -		   parser->opt_data.optarg);
> >> +    /* A long option.  Preserve the sign in the user key, without
> >> +       invoking undefined behavior.  Assume two's complement.  */
> >> +    {
> >> +      int user_key =
> >> +        ((opt & (1 << (USER_BITS - 1))) ? ~USER_MASK : 0) | (opt & USER_MASK);
> > 
> > Would this be clearer?
> > 
> > 		   (int) ((unsigned int) opt << GROUP_BITS) >> GROUP_BITS,
> 
> This would be another gnulib deviation, and the code comes from gnulib.
> I think we should keep it simple to make the sync more straightforward.
> 
> > 
> > Or does ubsan flag that as well?  Conversion to negative int is a GCC
> > extension:
> > 
> > | For conversion to a type of width N, the value is reduced modulo 2^N
> > | to be within range of the type; no signal is raised.
> 
> Yeah, but -fsanitize=undefinied still triggers this as UB.  I can add an
> option to suppress this kind of shift, but this will add a bit more
> complexity on the handler handling.

What kind of shift? In the variant suggested by Florian, left shift is done
in an unsigned type, that is well-defined. Neither Clang nor GCC instruments

int f(unsigned x)
{
    return (int)(x << 8) >> 8;
}

Alexander