Re: [PATCH 1/2] gdb: check for empty offsets vector in inherit_abstract_dies

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2022-10-21 11:35, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> +  if (!offsets.empty ())
> Simon> +    {
> Simon> +      std::sort (offsets.begin (), offsets.end ());
> Simon> +      sect_offset *offsets_end = offsets.data () + offsets.size ();
> Simon> +      for (sect_offset *offsetp = offsets.data () + 1;
> Simon> +	   offsetp < offsets_end;
> Simon> +	   offsetp++)
> Simon> +	if (offsetp[-1] == *offsetp)
> Simon> +	  complaint (_("Multiple children of DIE %s refer "
> Simon> +		       "to DIE %s as their abstract origin"),
> Simon> +		     sect_offset_str (die->sect_off),
> Simon> +		     sect_offset_str (*offsetp));
> Simon> +    }
>  
> Simon>    sect_offset *offsetp = offsets.data ();
> Simon> +  sect_offset *offsets_end = offsets.data () + offsets.size ();
> Simon>    die_info *origin_child_die = origin_die->child;
> Simon>    while (origin_child_die != nullptr && origin_child_die->tag != 0)
> Simon>      {
> 
> This subsequent loop does stuff like:
> 
>       while (offsetp < offsets_end
> 	     && *offsetp < origin_child_die->sect_off)
> 	offsetp++;
> 
> which in this scenario would be comparing null pointers using "<"?

Is it undefined behavior to compre two null pointers (both offsetp and
offset_end would be nullptr or both wouldn't be nullptr)?  At least
clang's UB sanitizer doesn't panic.

> I'm guessing this whole loop should be hoisted into the 'if'.

I don't understand enough what the code does to be convinced this would
be a correct change.

Note that the following patch changes this to use iterators, so it makes
the worry about comparing null pointers being undefined behavior go
away.  It becomes comparing offsets.end() with offsets.end().

Simon