[committed][gdb/symtab] Make find_block_in_blockvector more robust

[committed][gdb/symtab] Make find_block_in_blockvector more robust

[Tom de Vries]

Hi,

While working on PR25858 I noticed that the following trigger patch:
...
@@ -2918,6 +2918,7 @@ find_pc_sect_compunit_symtab
          const struct blockvector *bv;

          bv = COMPUNIT_BLOCKVECTOR (cust);
+         (volatile int)blockvector_contains_pc (bv, pc);
          b = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);

          if (BLOCK_START (b) <= pc
...
triggers this assert, which checks that the returned block indeed
contains pc:
...
@@ -170,7 +170,10 @@ find_block_in_blockvector

     {
       b = BLOCKVECTOR_BLOCK (bl, bot);
       if (BLOCK_END (b) > pc)
-       return b;
+       {
+         gdb_assert (BLOCK_START (b) <= pc);
+         return b;
+       }
       bot--;
     }

...
when running test-case gdb.ada/bp_c_mixed_case.exp.

It's possible that the trigger patch breaks an undocumented invariant:  I've
tried a build and test run without the trigger patch and did not manage to
trigger the assert.

For robustness-sake, fix the assert by bailing out if 'BLOCK_START (b) <= pc'
doesn't hold.

Tested on x86_64-linux.

Committed to trunk.

Thanks,
- Tom

[gdb/symtab] Make find_block_in_blockvector more robust

gdb/ChangeLog:

2020-10-22  Tom de Vries  <tdevries@suse.de>

	* block.c (find_block_in_blockvector): Make sure the returned block
	contains pc.

---
 gdb/block.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gdb/block.c b/gdb/block.c
index 597d6d5d87..070d3f7769 100644
--- a/gdb/block.c
+++ b/gdb/block.c
@@ -166,6 +166,8 @@ find_block_in_blockvector (const struct blockvector *bl, CORE_ADDR pc)
   while (bot >= STATIC_BLOCK)
     {
       b = BLOCKVECTOR_BLOCK (bl, bot);
+      if (!(BLOCK_START (b) <= pc))
+	return NULL;
       if (BLOCK_END (b) > pc)
 	return b;
       bot--;

Re: [committed][gdb/symtab] Make find_block_in_blockvector more robust

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> +      if (!(BLOCK_START (b) <= pc))
Tom> +	return NULL;

This seems a bit weird to me, in that if BLOCK_START(b) == pc, then I
would be inclined to say that the pc is in fact in that block.

Tom

Re: [committed][gdb/symtab] Make find_block_in_blockvector more robust

[Tom de Vries]

On 10/22/20 8:56 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> +      if (!(BLOCK_START (b) <= pc))
> Tom> +	return NULL;
> 
> This seems a bit weird to me, in that if BLOCK_START(b) == pc, then I
> would be inclined to say that the pc is in fact in that block.
> 

So if BLOCK_START(b) == pc, indeed the pc is in the block, and we have:
...
      if (!(true))
	return NULL;
...
which I'd say correctly handles that case.

Thanks,
- Tom

Re: [committed][gdb/symtab] Make find_block_in_blockvector more robust

[Simon Marchi]

On 2020-10-22 5:21 p.m., Tom de Vries wrote:
> On 10/22/20 8:56 PM, Tom Tromey wrote:
>>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>>
>> Tom> +      if (!(BLOCK_START (b) <= pc))
>> Tom> +	return NULL;
>>
>> This seems a bit weird to me, in that if BLOCK_START(b) == pc, then I
>> would be inclined to say that the pc is in fact in that block.
>>
>
> So if BLOCK_START(b) == pc, indeed the pc is in the block, and we have:
> ...
>       if (!(true))
> 	return NULL;
> ...
> which I'd say correctly handles that case.
>
> Thanks,
> - Tom
>

I think that turning:

  if (!(BLOCK_START (b) <= pc))

into

  if (BLOCK_START (b) > pc)

or

  if (pc < BLOCK_START (b))

would make it easier to read.

Simon

Re: [committed][gdb/symtab] Make find_block_in_blockvector more robust

[Tom de Vries]

On 10/22/20 11:42 PM, Simon Marchi wrote:
> On 2020-10-22 5:21 p.m., Tom de Vries wrote:
>> On 10/22/20 8:56 PM, Tom Tromey wrote:
>>>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>>>
>>> Tom> +      if (!(BLOCK_START (b) <= pc))
>>> Tom> +	return NULL;
>>>
>>> This seems a bit weird to me, in that if BLOCK_START(b) == pc, then I
>>> would be inclined to say that the pc is in fact in that block.
>>>
>>
>> So if BLOCK_START(b) == pc, indeed the pc is in the block, and we have:
>> ...
>>       if (!(true))
>> 	return NULL;
>> ...
>> which I'd say correctly handles that case.
>>
>> Thanks,
>> - Tom
>>
> 
> I think that turning:
> 
>   if (!(BLOCK_START (b) <= pc))
> 
> into
> 
>   if (BLOCK_START (b) > pc)
> 
> or
> 
>   if (pc < BLOCK_START (b))
> 
> would make it easier to read.

Thanks for the feedback.

I agree it would make it easier to read, but I'm afraid it would make it
harder to understand.

In general I prefer range tests to conform to this layout:
...
min cmp-op-1 val && val cmp-op-2 max
...
which IMO is the best approximation of the non-C:
...
min cmp-op-1 val cmp-op-2 max
...
and then use the range expression as a whole, either negated or not.

But indeed, the negated case is a bit harder to read, something I
sometimes fix by using a variable with a somewhat helpful name.

This follows my preferred layout for the range expression (albeit split
up into two variables), while not negating non-trivial expressions:
...
  while (bot >= STATIC_BLOCK)
    {
      b = BLOCKVECTOR_BLOCK (bl, bot);
      bool start_ok = BLOCK_START (b) <= pc;
      bool end_ok = pc < BLOCK_END (b);
      if (!start_ok)
        return NULL;
      if (end_ok)
        return b;
      bot--;
    }
...
but perhaps this is making things less clear?  WDYT?

Thanks,
- Tom

Re: [committed][gdb/symtab] Make find_block_in_blockvector more robust

[Simon Marchi]

On 2020-10-23 10:08 a.m., Tom de Vries wrote:
> Thanks for the feedback.
>
> I agree it would make it easier to read, but I'm afraid it would make it
> harder to understand.
>
> In general I prefer range tests to conform to this layout:
> ...
> min cmp-op-1 val && val cmp-op-2 max
> ...
> which IMO is the best approximation of the non-C:
> ...
> min cmp-op-1 val cmp-op-2 max
> ...
> and then use the range expression as a whole, either negated or not.

I think we all picture things a bit differently in our minds, which
means we prefer some form over the other, and that's fine.

Not trying to convince you to change anything, but just for the sake of
discussion, this is how I picture it.  When reading conditions, I
translate to natural language in my head.  Between:

  if the pc is smaller than the start of the block, then abort

and

  if the start of the block is not smaller or equal than the pc

In the first case, I understand in an instant "ok, it means if the pc is
before the range". For the second, it takes me a little pause: "ok, so
if the start of the block is smaller or equal to the pc, it means we're
in range, so if _not_ that, it means we're out of range.".

>
> But indeed, the negated case is a bit harder to read, something I
> sometimes fix by using a variable with a somewhat helpful name.
>
> This follows my preferred layout for the range expression (albeit split
> up into two variables), while not negating non-trivial expressions:
> ...
>   while (bot >= STATIC_BLOCK)
>     {
>       b = BLOCKVECTOR_BLOCK (bl, bot);
>       bool start_ok = BLOCK_START (b) <= pc;
>       bool end_ok = pc < BLOCK_END (b);
>       if (!start_ok)
>         return NULL;
>       if (end_ok)
>         return b;
>       bot--;
>     }
> ...

I like this version with variable names.  The variable names convey some
information about the intention of the code, and act as some kind of
"checkpoints".  If you understand what these boolean variables are meant
to represent (hence the name must be clear), you don't need to parse the
condition themselves to understand the algorithm.  If you have some
reason to believe the conditions are erroneous, then you can dig into
them, but otherwise you can skip them and save some mental cycles.  Of
course, it gets more valuable as the size/complexity of the conditions
increase.

Simon

Re: [committed][gdb/symtab] Make find_block_in_blockvector more robust

[Tom Tromey]

>> This seems a bit weird to me, in that if BLOCK_START(b) == pc, then I
>> would be inclined to say that the pc is in fact in that block.

Tom> So if BLOCK_START(b) == pc, indeed the pc is in the block, and we have:
Tom> ...
Tom>       if (!(true))
Tom> 	return NULL;
Tom> ...
Tom> which I'd say correctly handles that case.

I guess I missed the "!".  Thank you.

Tom