[009/203] what is this code for

Message ID 20210101214723.1784144-10-tom@tromey.com
State New
Headers show
Series
  • Refactor expressions
Related show

Commit Message

Tom Tromey Jan. 1, 2021, 9:44 p.m.
---
 gdb/eval.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.26.2

Comments

Joel Brobecker Jan. 3, 2021, 6 a.m. | #1
Hi Tom,

> @@ -1224,7 +1224,7 @@ eval_op_var_msym_value (struct type *expect_type, struct expression *exp,

>  

>    struct type *type = value_type (val);

>    if (type->code () == TYPE_CODE_ERROR

> -      && (noside != EVAL_AVOID_SIDE_EFFECTS || pc != 0))

> +      && (noside != EVAL_AVOID_SIDE_EFFECTS))

>      error_unknown_type (msymbol->print_name ());

>    return val;


I tracked the origin of this code down to the following commit...

    commit 46a4882b3c7d9ec981568b8b13a3c9c39c8f8e61
    Author: Pedro Alves <palves@redhat.com>
    Date:   Mon Sep 4 20:21:15 2017 +0100
    Subject: Stop assuming no-debug-info variables have type int

... which was the second stage of a similar change for functions.
The revision log did not really give an explicit explanation for
this code, but after a bit of thinking, I think I was able to
figure it out.

Consider the case where you're trying to investigate a variable
called "opaque" which is declared in the symbol table but does not
have any debug info.

  * If noside != EVAL_AVOID_SIDE_EFFECTS (typically, we're trying
    to print the value, then we always error out with:

        | 'opaque' has unknown type; cast it to its declared type

    ... This is what the patch is about. That's not what you are
    asking about, because it's easy to understand. OK.

  * So, if noside == EVAL_AVOID_SIDE_EFFECTS, what does it mean
    to only print the error message above if pc != 0?

    Well, if pc == 0, it means OP_VAR_MSYM_VALUE node is first,
    and since that operator doesn't have any "parameters", what
    it means to me is that it must actually be the only operand
    of the expression. Thus, we must be facing the following
    situation:

        | (gdb) ptype opaque

    And what do we do in that scenario? Well, we have code in gdbtype.c
    which gives default types for the various kinds of minimal symbols.
    For data symbols, it looks like this:

        | objfile_type->nodebug_data_symbol
        |   = init_nodebug_var_type (objfile, "<data variable, no debug info>");

    So, when we do a ptype, we don't get an error, but instead, get
    some information about the symbol:

        | (gdb) ptype opaque
        | type = <data variable, no debug info>


    If, on the other hand pc != 0, it means our minsym is part
    of a larger expression, such as, for instance "opaque + 1".
    With today's GDB, a "ptype" of the expression above generates
    the same error message when when in EVAL_NORMAL mode:

        | (gdb) ptype opaque + 1
        | 'opaque' has unknown type; cast it to its declared type

    If you remove the check above, then we no longer trigger
    the "has unknown type" error. So, instead, we just evaluate
    the minsym as is, which means we get a value whose type is
    the type of "objfile_type->nodebug_data_symbol", in other words
    TYPE_CODE_ERROR, which leads to:

        | (gdb) ptype opaque + 1
        | Argument to arithmetic operation not a number or boolean.

    For minimal symbols which are text symbols, these get a default
    type of TYPE_CODE_FUNC, so removing the above breaks the check
    which forces the user to specify the return type.

We should probably add a test or two in gdb.base/nodebug.exp for that...

-- 
Joel
Tom de Vries via Gdb-patches Jan. 25, 2021, 2:28 a.m. | #2
On 2021-01-03 1:00 a.m., Joel Brobecker wrote:
> Hi Tom,

> 

>> @@ -1224,7 +1224,7 @@ eval_op_var_msym_value (struct type *expect_type, struct expression *exp,

>>  

>>    struct type *type = value_type (val);

>>    if (type->code () == TYPE_CODE_ERROR

>> -      && (noside != EVAL_AVOID_SIDE_EFFECTS || pc != 0))

>> +      && (noside != EVAL_AVOID_SIDE_EFFECTS))

>>      error_unknown_type (msymbol->print_name ());

>>    return val;

> 

> I tracked the origin of this code down to the following commit...

> 

>     commit 46a4882b3c7d9ec981568b8b13a3c9c39c8f8e61

>     Author: Pedro Alves <palves@redhat.com>

>     Date:   Mon Sep 4 20:21:15 2017 +0100

>     Subject: Stop assuming no-debug-info variables have type int

> 

> ... which was the second stage of a similar change for functions.

> The revision log did not really give an explicit explanation for

> this code, but after a bit of thinking, I think I was able to

> figure it out.

> 

> Consider the case where you're trying to investigate a variable

> called "opaque" which is declared in the symbol table but does not

> have any debug info.

> 

>   * If noside != EVAL_AVOID_SIDE_EFFECTS (typically, we're trying

>     to print the value, then we always error out with:

> 

>         | 'opaque' has unknown type; cast it to its declared type

> 

>     ... This is what the patch is about. That's not what you are

>     asking about, because it's easy to understand. OK.

> 

>   * So, if noside == EVAL_AVOID_SIDE_EFFECTS, what does it mean

>     to only print the error message above if pc != 0?

> 

>     Well, if pc == 0, it means OP_VAR_MSYM_VALUE node is first,

>     and since that operator doesn't have any "parameters", what

>     it means to me is that it must actually be the only operand

>     of the expression. Thus, we must be facing the following

>     situation:

> 

>         | (gdb) ptype opaque

> 

>     And what do we do in that scenario? Well, we have code in gdbtype.c

>     which gives default types for the various kinds of minimal symbols.

>     For data symbols, it looks like this:

> 

>         | objfile_type->nodebug_data_symbol

>         |   = init_nodebug_var_type (objfile, "<data variable, no debug info>");

> 

>     So, when we do a ptype, we don't get an error, but instead, get

>     some information about the symbol:

> 

>         | (gdb) ptype opaque

>         | type = <data variable, no debug info>

> 

> 

>     If, on the other hand pc != 0, it means our minsym is part

>     of a larger expression, such as, for instance "opaque + 1".

>     With today's GDB, a "ptype" of the expression above generates

>     the same error message when when in EVAL_NORMAL mode:

> 

>         | (gdb) ptype opaque + 1

>         | 'opaque' has unknown type; cast it to its declared type

> 

>     If you remove the check above, then we no longer trigger

>     the "has unknown type" error. So, instead, we just evaluate

>     the minsym as is, which means we get a value whose type is

>     the type of "objfile_type->nodebug_data_symbol", in other words

>     TYPE_CODE_ERROR, which leads to:

> 

>         | (gdb) ptype opaque + 1

>         | Argument to arithmetic operation not a number or boolean.

> 

>     For minimal symbols which are text symbols, these get a default

>     type of TYPE_CODE_FUNC, so removing the above breaks the check

>     which forces the user to specify the return type.

> 

> We should probably add a test or two in gdb.base/nodebug.exp for that...

> 


In addition to this good explanation: I tried removing the `|| pc == 0`
from the code (based on current master) and got these failures:

+FAIL: gdb.base/nodebug.exp: p sizeof(dataglobal + 1)
+FAIL: gdb.base/nodebug.exp: ptype *dataglobal
+FAIL: gdb.base/nodebug.exp: ptype dataglobal + 1
+FAIL: gdb.base/nodebug.exp: ptype sizeof(dataglobal + 1)
+FAIL: gdb.base/nodebug.exp: whatis *dataglobal
+FAIL: gdb.base/nodebug.exp: whatis dataglobal + 1
+FAIL: gdb.base/nodebug.exp: whatis sizeof(dataglobal + 1)

So it's already covered.  Tom, are these cases correctly handled with
this series applied?

Simon
Tom Tromey Jan. 25, 2021, 3:27 a.m. | #3
Simon> So it's already covered.  Tom, are these cases correctly handled with
Simon> this series applied?

I regression-tested it and compared and it passed; however I don't think
this is explicitly handled, so I suspect that the comparison was wrong.
Maybe this will need a new method on operation to be handled correctly.
I'll take a look when I rebase the series and fix up the other comments.

Tom
Tom Tromey Feb. 11, 2021, 2:25 a.m. | #4
>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:


Simon> In addition to this good explanation: I tried removing the `|| pc == 0`
Simon> from the code (based on current master) and got these failures:

Simon> +FAIL: gdb.base/nodebug.exp: p sizeof(dataglobal + 1)
Simon> +FAIL: gdb.base/nodebug.exp: ptype *dataglobal
Simon> +FAIL: gdb.base/nodebug.exp: ptype dataglobal + 1
Simon> +FAIL: gdb.base/nodebug.exp: ptype sizeof(dataglobal + 1)
Simon> +FAIL: gdb.base/nodebug.exp: whatis *dataglobal
Simon> +FAIL: gdb.base/nodebug.exp: whatis dataglobal + 1
Simon> +FAIL: gdb.base/nodebug.exp: whatis sizeof(dataglobal + 1)

Simon> So it's already covered.  Tom, are these cases correctly handled with
Simon> this series applied?

It wasn't, which I don't understand, since I regression tested this.
I wonder if the comparison script I am using is buggy.

Anyway, I have reimplemented this feature in the new version of the
series, these tests work now.  There's one more failure in nodebug.exp
that I have yet to track down - in one case, the error message has
changed.

Tom
Tom Tromey Feb. 13, 2021, 7:37 p.m. | #5
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:


Tom> There's one more failure in nodebug.exp
Tom> that I have yet to track down - in one case, the error message has
Tom> changed.

I've fixed this now as well.

Tom

Patch

diff --git a/gdb/eval.c b/gdb/eval.c
index b00139ca36f..f2507f25f44 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -1224,7 +1224,7 @@  eval_op_var_msym_value (struct type *expect_type, struct expression *exp,
 
   struct type *type = value_type (val);
   if (type->code () == TYPE_CODE_ERROR
-      && (noside != EVAL_AVOID_SIDE_EFFECTS || pc != 0))
+      && (noside != EVAL_AVOID_SIDE_EFFECTS))
     error_unknown_type (msymbol->print_name ());
   return val;
 }