Re: [patch] Fix a crash when displaying variables from shared library.

[Paul Pluzhnikov <ppluzhnikov at google dot com>]

On Sun, Feb 22, 2009 at 5:07 PM, Joel Brobecker <brobecker@adacore.com> wrote:

>> +       /* Can't re-parse the expression. Disable this display item. */
>
> Minor style issue: You need to have two spaces after each period
> (one in the middle, and one at the end, before the "*/").

Done.

>> +/* Answer 1 if "d" uses "solib" (and will become dangling when "solib"
>> +   is unloaded), otherwise answer 0.  */
>
> If you don't mind, I think using "Return 1" instead of "Answer 1" would
> be more consistent with the other descriptions.

Done.

> Another minor style correction: In GDB, we refer to the function
> parameters by using their names in ALL_CAPS, and without the quotes.
> So, in your case, you would write:
>
> /* Return 1 if D uses SOLIB (and will become dangling [...] */

Done.

>> +  if (d->block != NULL
>> +      && addr_low <= d->block->startaddr && d->block->endaddr <= addr_high)
>> +    return 1;
>
> I suggest you use solib_address instead of doing the check yourself.
> As mentioned by Daniel in another thread, shared libraries on SVR4
> systems occupy a contiguous address block, but this is not the case
> of DLLs where the data and text sections might be separate.
> I verified that solib_address should handle the DLL case.

Done.

>> +  for (i = 0; i < d->exp->nelts; i++)
>> +    {
>> +      union exp_element *elts = d->exp->elts;
>> +      if (elts[i].opcode == OP_VAR_VALUE)
>
> I'm afraid this isn't going to work for more complex structures...
> The problem is that you might be reading an undefined field of
> union exp_element. Imagine for instance that you have an expression
> that looks like this: "foo->bar".
>
> At one point, you'll encounter the following elements:
>
>   [i  ]  ->  STRUCTOP_PTR
>   [i+1]  ->  A string
>   [i+2]  ->  STRUCTOP_PTR
>
> Iterating over the expression, you'll ignore the element at index i,
> and then check the opcode of the element at i+1, which is the wrong
> field of the enum to access in this case...

I was afraid of that ...

> I can't think of a way of doing what you're trying to do off the top
> of my head. I'll have to think about it a little more. Perhaps others
> will have suggestions... Or perhaps we'll have to attack the problem
> with a different angle, I'm not very familiar with how "display"
> expressions are handled...

One way I think this could be fixed is to refactor
e.g. dump_subexp_body_standard() to use "visitor pattern", and then
use a different callback for display_uses_solib_p().

Perhaps refactoring should be done first as a separate patch?

A simpler way is to cut/paste/modify dump_subexp_body_standard(),
but there is (IMHO) already too much code duplication between it
and e.g. evaluate_subexp_standard(), so I'd rather not introduce
a 3rd copy.

Another way would be to replace malloc()/realloc()s in
parse_exp_in_context() with calloc()s, so that all "unused" .opcode's
become OP_NULLs. This appears tricky, and there are language hooks
which may need similar fixes.


>> +gdb_test "run" "3: c_global = 43\\r\\nwarning: .*b_global.*\\r\\n1: a_global = 41" "after rebuild"
>
> Can this be changed to use either one of the runto routines, or maybe
> gdb_run_cmd if one of the above doesn't work in this case?

Note that this test is skipped when "is_remote target".
Is there still a need to use runto?

Is it better to use runto and *not* skip the test for remote targets?

>> +gdb_test "run" "6: a_static = 46\\r\\n4: main_global = 44\\r\\n.*"
>
> Same here.

Thanks,
-- 
Paul Pluzhnikov