Re: [PATCH 1/2] gdb: Avoid undefined shifts

[Pedro Alves <pedro@palves.net>]

Reading this back, I noticed a few typos, pointed out below.  I'll send a v2 shortly
with these fixed...

On 2022-04-01 21:22, Pedro Alves wrote:

> - Make GDB warn for the cases that are still undefined in C++20.  The
>   warnings' texts are same as what GCC prints.

"are the same"

> +# Test a print command that prints out RESULT_RE.  If WARNING is
> +# non-empty, it is expected that GDB prints this warning before the
> +# print result.  If WARNING is empty, it is expected that GDB prints
> +# text other than the print result.

"that GDB prints text other" -> "that GDB prints no text other"

> +proc test_shift {cmd result_re {warning ""}} {
> +    if {$warning != ""} {
> +	set warning_re "[string_to_regexp $warning]\r\n"
> +    } else {
> +	set warning_re ""
> +    }
> +
> +    set cmd_re [string_to_regexp $cmd]
> +
> +    gdb_test_multiple $cmd "" {
> +	-re -wrap "^$cmd_re\r\n$warning_re\\$$::decimal$result_re" {
> +	    pass $gdb_test_name
> +	}
> +    }
> +}
> +
> +# Test a print command that issues a warning about the right shift
> +# count being negative.  The "ne" suffix stands for negative.
> +proc test_shift_ne {cmd result} {
> +    test_shift $cmd $result "warning: right shift count is negative"
> +}
> +
> +# Test a print command that issues a warning about the right shift
> +# count being too large.  The "lt" suffix stands for "too large".

Typo "lt" -> "tl"...

> +proc test_shift_tl {cmd result} {
> +    test_shift $cmd $result "warning: right shift count >= width of type"
> +}

Meanwhile I realized it's simpler to store the warning texts in variables and
use those, instead of these wrapper procedures.

> +/* Check whether the the RHS value of a shift is valid.  RHS_VAL is

Double "the the".

> +   the shift amount, and RESULT_TYPE is result type.  This is used to

"is the result"

> +   avoid both negative or too-large shift amounts, which are

or -> and

> +   undefined, and would crash a GDB built with UBSan.  Warns and
> +   returns false if not valid, and returns true if not valid.  */
> +


>  	    case BINOP_LSH:
> -	      v = v1 << v2;
> +	      if (!check_valid_shift_rhs (result_type, v2))
> +		v = 0;
> +	      else
> +		{
> +		  /* Cast to unsigned to avoid undefined behavior on
> +		     signed shift overflow (unless C++20 or later),
> +		     which would crash GDB when built with UBSan.
> +		     Note We don't warn on left signed shift overflow,

"Note We" -> "Note we"

> +		     because starting with C++20, that is actually
> +		     defined behavior.  */
> +		  v = (ULONGEST) v1 << v2;
> +		}
>  	      break;
>  
>  	    case BINOP_RSH:
> -	      v = v1 >> v2;
> +	      if (!check_valid_shift_rhs (result_type, v2))
> +		v = 0;
> +	      else
> +		v = v1 >> v2;
>  	      break;
>  
>  	    case BINOP_BITWISE_AND: