[PATCH] Fix number of children of varobj with stub debug info

[PATCH] Fix number of children of varobj with stub debug info

[Christian Walther]

GDB under some circumstances wrongly reports 'numchild="0"' on a
variable object for a pointer to a C++ class instance when the type
description from the debug info is a stub that does not include members.
The code path is missing a call to check_typedef() to look up the
complete type description. This commit adds it.

Additional conditions for the bug to occur:
1. "print object" is set to "on"
2. The class has no run-time type identification (no vtable).

Debug information of this kind is generated by Clang 13.
---

Note: I am not certain whether this is the best place to insert the
missing call, or whether placing it somewhere else would benefit other
code paths too. It does fix my particular problem where I put it.


Steps to reproduce:

1. Create the following three files:

main.cpp
----------------
#include <cstdio>
#include "c.h"

extern "C" int main(int argc, char** argv) {
	C* c = new C(5);
	printf("Hello World! %d\n", c->getX());
	return 0;
}
----------------

c.h
----------------
class C {
	public:
		C(int ax);
		int getX();
	private:
		int x;
};
----------------

c.cpp
----------------
#include "c.h"

C::C(int ax) : x(ax) {}

int C::getX() {
	return x;
}
----------------

2. Compile them with Clang 13:

clang+llvm-13.0.1-x86_64-linux-gnu-ubuntu-18.04/bin/clang++ -g -O0 -c -o main.o main.cpp
clang+llvm-13.0.1-x86_64-linux-gnu-ubuntu-18.04/bin/clang++ -g -O0 -c -o c.o c.cpp
clang+llvm-13.0.1-x86_64-linux-gnu-ubuntu-18.04/bin/clang++ -g -o mini main.o c.o

3. Verify that the debug information of main.o only contains a stub
definition of class C that does not include member x:

readelf --debug-dump=info main.o

It should look roughly like this and not contain any DW_TAG_member:

 <1><690>: Abbrev Number: 23 (DW_TAG_class_type)
    <691>   DW_AT_name        : (indirect string, offset: 0x265): C
    <695>   DW_AT_declaration : 1

(I have not been able to reproduce this with
clang+llvm-12.0.1-x86_64-linux-gnu-ubuntu-16.04, which generates a full
definition, and clang+llvm-14.0.0-x86_64-linux-gnu-ubuntu-18.04, which
generates invalid debug info.)

4. Debug:

gdb --interpreter mi2 --args mini
-break-insert main.cpp:6
-gdb-set print object on
-exec-run
-var-create - * c
-gdb-exit

Expected result:

^done,name="var1",numchild="1",value="0x...",type="C *",thread-id="1",has_more="0"

Actual result:

^done,name="var1",numchild="0",value="0x...",type="C *",thread-id="1",has_more="0"

---
 gdb/c-varobj.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdb/c-varobj.c b/gdb/c-varobj.c
index 8fecbd57e08..0c0b1abe968 100644
--- a/gdb/c-varobj.c
+++ b/gdb/c-varobj.c
@@ -121,6 +121,7 @@ adjust_value_for_child_access (struct value **value,
       enclosing_type = value_actual_type (*value, 1, &real_type_found);
       if (real_type_found)
 	{
+	  enclosing_type = check_typedef(enclosing_type);
 	  *type = enclosing_type;
 	  *value = value_cast (enclosing_type, *value);
 	}
-- 
2.25.1

Re: [PATCH] Fix number of children of varobj with stub debug info

[Bruno Larsen]

On 6/21/22 05:18, Christian Walther wrote:
> GDB under some circumstances wrongly reports 'numchild="0"' on a
> variable object for a pointer to a C++ class instance when the type
> description from the debug info is a stub that does not include members.
> The code path is missing a call to check_typedef() to look up the
> complete type description. This commit adds it.
> 
> Additional conditions for the bug to occur:
> 1. "print object" is set to "on"
> 2. The class has no run-time type identification (no vtable).

Hi! Thanks for looking at this!

The mention to "print object on" looks very weird to me. The fact that
without it GDB works fine makes me think that the different code path
should be fixed.

Looking at the comments, c-varobj.c:111 says that:

/* The 'get_target_type' function calls check_typedef on
    result, so we can immediately check type code.  No
    need to call check_typedef here.  */

So I'd say that going specifically against this comment is not a good idea.
My guess for a better solution would be checking if the type we already had
(either from line 75 or line 105) matches the enclosing type, in which case
we would keep the previous variable, or check which is more complete. Sorry
I can't give more help, I'm still new to GDB.

Also, for a version 2, it would be nice if you could provide a test case.
We have a mechanism for generating custom DWARF information for compiled
programs, called dwarf assembler(sorry if you knew this already). This way
we can ensure GDB never regresses.

Finally, please be sure to check the contribution checklist (https://sourceware.org/gdb/wiki/ContributionChecklist).
You must add a whitespace before the ( for function calls.


Cheers!
Bruno Larsen
> 
> Debug information of this kind is generated by Clang 13.
> ---
> 
> Note: I am not certain whether this is the best place to insert the
> missing call, or whether placing it somewhere else would benefit other
> code paths too. It does fix my particular problem where I put it.
> 
> 
> Steps to reproduce:
> 
> 1. Create the following three files:
> 
> main.cpp
> ----------------
> #include <cstdio>
> #include "c.h"
> 
> extern "C" int main(int argc, char** argv) {
> 	C* c = new C(5);
> 	printf("Hello World! %d\n", c->getX());
> 	return 0;
> }
> ----------------
> 
> c.h
> ----------------
> class C {
> 	public:
> 		C(int ax);
> 		int getX();
> 	private:
> 		int x;
> };
> ----------------
> 
> c.cpp
> ----------------
> #include "c.h"
> 
> C::C(int ax) : x(ax) {}
> 
> int C::getX() {
> 	return x;
> }
> ----------------
> 
> 2. Compile them with Clang 13:
> 
> clang+llvm-13.0.1-x86_64-linux-gnu-ubuntu-18.04/bin/clang++ -g -O0 -c -o main.o main.cpp
> clang+llvm-13.0.1-x86_64-linux-gnu-ubuntu-18.04/bin/clang++ -g -O0 -c -o c.o c.cpp
> clang+llvm-13.0.1-x86_64-linux-gnu-ubuntu-18.04/bin/clang++ -g -o mini main.o c.o
> 
> 3. Verify that the debug information of main.o only contains a stub
> definition of class C that does not include member x:
> 
> readelf --debug-dump=info main.o
> 
> It should look roughly like this and not contain any DW_TAG_member:
> 
>   <1><690>: Abbrev Number: 23 (DW_TAG_class_type)
>      <691>   DW_AT_name        : (indirect string, offset: 0x265): C
>      <695>   DW_AT_declaration : 1
> 
> (I have not been able to reproduce this with
> clang+llvm-12.0.1-x86_64-linux-gnu-ubuntu-16.04, which generates a full
> definition, and clang+llvm-14.0.0-x86_64-linux-gnu-ubuntu-18.04, which
> generates invalid debug info.)
> 
> 4. Debug:
> 
> gdb --interpreter mi2 --args mini
> -break-insert main.cpp:6
> -gdb-set print object on
> -exec-run
> -var-create - * c
> -gdb-exit
> 
> Expected result:
> 
> ^done,name="var1",numchild="1",value="0x...",type="C *",thread-id="1",has_more="0"
> 
> Actual result:
> 
> ^done,name="var1",numchild="0",value="0x...",type="C *",thread-id="1",has_more="0"
> 
> ---
>   gdb/c-varobj.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/gdb/c-varobj.c b/gdb/c-varobj.c
> index 8fecbd57e08..0c0b1abe968 100644
> --- a/gdb/c-varobj.c
> +++ b/gdb/c-varobj.c
> @@ -121,6 +121,7 @@ adjust_value_for_child_access (struct value **value,
>         enclosing_type = value_actual_type (*value, 1, &real_type_found);
>         if (real_type_found)
>   	{
> +	  enclosing_type = check_typedef(enclosing_type);
>   	  *type = enclosing_type;
>   	  *value = value_cast (enclosing_type, *value);
>   	}

Re: [PATCH] Fix number of children of varobj with stub debug info

[Christian Walther]

Thanks for the quick reaction and for your comments!

> The mention to "print object on" looks very weird to me. The fact that
> without it GDB works fine makes me think that the different code path
> should be fixed.
> 
> Looking at the comments, c-varobj.c:111 says that:
> 
> /* The 'get_target_type' function calls check_typedef on
>   result, so we can immediately check type code.  No
>   need to call check_typedef here.  */
> 
> So I'd say that going specifically against this comment is not a good idea.

The dependency on "print object on" comes from the "if" on c-varobj.c:575: when "print object" is off, adjust_value_for_child_access() is called without passing it the value, it doesn't enter the "if" on c-varobj.c:116, and the return value from get_target_type() is returned, which as the mentioned comment says already had check_typedef() called on it, so all is well.

That comment however is irrelevant if the "if" on line 116 is taken, because in that case the return value from get_target_type() is discarded (it was only used to convert from the pointer value to the pointed-to value) and replaced by the type extracted from the value, which has not had check_typedef() called on it (until I added it explicitly). So, I don't think I am "going specifically against this comment".

(In fact, I think that comment is in a strange place. It would make more sense if it was between lines 88 and 89. It was introduced in 2024f65, when it was at least a little closer.)

> My guess for a better solution would be checking if the type we already had
> (either from line 75 or line 105) matches the enclosing type, in which case
> we would keep the previous variable, or check which is more complete.

I'm not sure I understand what you mean by this. The type we get on line 75, in the case where the bug occurs, is the pointer type and therefore definitely doesn't match the enclosing type. The type we get on line 105 does match (in the sense of "describes the same type", not "==") the enclosing type in my example, because class C does not have any super-/subclasses and declared type and runtime type are the same for variable c, but that need not necessarily be the case. When it is not, then couldn't the bug still occur unless we add a call to check_typedef()? Are we guaranteed that the enclosing type is not a stub (has already had check_typedef() called) in that case? Even if we were, isn't it still simpler and less error-prone to unconditionally add a call to check_typedef() on the output of value_actual_type(), as I proposed, given that as far as I can tell it is a harmless no-op when it doesn't have to do anything?

> Also, for a version 2, it would be nice if you could provide a test case.
> We have a mechanism for generating custom DWARF information for compiled
> programs, called dwarf assembler(sorry if you knew this already). This way
> we can ensure GDB never regresses.

I had thought about that, but had no idea where to even start to build a test case that doesn't require Clang 13 and runs on any architecture. Your mention of a "dwarf assembler" (I did not know about that) gives me a clue, that sounds useful. I have now searched for that term and found it in a couple of places, but it still all looks like gobbledygook to me. I fear I won't have time to learn enough about DejaGnu or whatever it is to get such a thing to work. I can't spend more than another day or two on this. I'm sorry but I fear I'll have to offer the patch without a test case, take it or leave it. I don't mind if you decline, we build GDB ourselves with a couple of patches anyway and I can just apply the patch there to solve my problem.

> You must add a whitespace before the ( for function calls.

Whoops, I thought I had checked the code style, but that slipped through. Thanks, will fix in v2.

In summary, I am unsure about how to proceed. Unless you can give me more guidance on what a better solution would be, I will just resubmit the same patch with the style issue fixed.

 -Christian

Re: [PATCH] Fix number of children of varobj with stub debug info

[Bruno Larsen]

On 6/22/22 09:58, Christian Walther wrote:
> Thanks for the quick reaction and for your comments!
> 
>> The mention to "print object on" looks very weird to me. The fact that
>> without it GDB works fine makes me think that the different code path
>> should be fixed.
>>
>> Looking at the comments, c-varobj.c:111 says that:
>>
>> /* The 'get_target_type' function calls check_typedef on
>>    result, so we can immediately check type code.  No
>>    need to call check_typedef here.  */
>>
>> So I'd say that going specifically against this comment is not a good idea.
> 
> The dependency on "print object on" comes from the "if" on c-varobj.c:575: when "print object" is off, adjust_value_for_child_access() is called without passing it the value, it doesn't enter the "if" on c-varobj.c:116, and the return value from get_target_type() is returned, which as the mentioned comment says already had check_typedef() called on it, so all is well.
> 
> That comment however is irrelevant if the "if" on line 116 is taken, because in that case the return value from get_target_type() is discarded (it was only used to convert from the pointer value to the pointed-to value) and replaced by the type extracted from the value, which has not had check_typedef() called on it (until I added it explicitly). So, I don't think I am "going specifically against this comment".
> 
> (In fact, I think that comment is in a strange place. It would make more sense if it was between lines 88 and 89. It was introduced in 2024f65, when it was at least a little closer.)

Ah, thanks for this. I should have checked the original commit.

Given this confusion, I think we should either move to line 89, where you mentioned, or just remove the comments altogether.

> 
>> My guess for a better solution would be checking if the type we already had
>> (either from line 75 or line 105) matches the enclosing type, in which case
>> we would keep the previous variable, or check which is more complete.
> 
> I'm not sure I understand what you mean by this. The type we get on line 75, in the case where the bug occurs, is the pointer type and therefore definitely doesn't match the enclosing type. The type we get on line 105 does match (in the sense of "describes the same type", not "==") the enclosing type in my example, because class C does not have any super-/subclasses and declared type and runtime type are the same for variable c, but that need not necessarily be the case. When it is not, then couldn't the bug still occur unless we add a call to check_typedef()? Are we guaranteed that the enclosing type is not a stub (has already had check_typedef() called) in that case? Even if we were, isn't it still simpler and less error-prone to unconditionally add a call to check_typedef() on the output of value_actual_type(), as I proposed, given that as far as I can tell it is a harmless no-op when it doesn't have to do anything?

Yeah, with the updated context, I agree more with your solution. What I wanted was to think of something cheaper, but you make a good point.

> 
>> Also, for a version 2, it would be nice if you could provide a test case.
>> We have a mechanism for generating custom DWARF information for compiled
>> programs, called dwarf assembler(sorry if you knew this already). This way
>> we can ensure GDB never regresses.
> 
> I had thought about that, but had no idea where to even start to build a test case that doesn't require Clang 13 and runs on any architecture. Your mention of a "dwarf assembler" (I did not know about that) gives me a clue, that sounds useful. I have now searched for that term and found it in a couple of places, but it still all looks like gobbledygook to me. I fear I won't have time to learn enough about DejaGnu or whatever it is to get such a thing to work. I can't spend more than another day or two on this. I'm sorry but I fear I'll have to offer the patch without a test case, take it or leave it. I don't mind if you decline, we build GDB ourselves with a couple of patches anyway and I can just apply the patch there to solve my problem.

Well, I don't have the power to accept anything, but I do think that a fix without test is better than no fix at all. Someone can probably do this in the near future, if you can't

> 
>> You must add a whitespace before the ( for function calls.
> 
> Whoops, I thought I had checked the code style, but that slipped through. Thanks, will fix in v2.
> 
> In summary, I am unsure about how to proceed. Unless you can give me more guidance on what a better solution would be, I will just resubmit the same patch with the style issue fixed.

Sounds good, sorry about the run-around. Here's hoping someone with approval permissions looks at your v2

Cheers!
Bruno Larsen

> 
>   -Christian
> 

[PATCH v2 1/2] Fix number of children of varobj with stub debug info

[Christian Walther]

GDB under some circumstances wrongly reports 'numchild="0"' on a
variable object for a pointer to a C++ class instance when the type
description from the debug info is a stub that does not include members.
The code path is missing a call to check_typedef() to look up the
complete type description. This commit adds it.

Additional conditions for the bug to occur:
1. "print object" is set to "on"
2. The class has no run-time type identification (no vtable).

Debug information of this kind is generated by Clang 13.
---

Changes in v2:
- fix code style 
- add second commit to move confusing comment

---
 gdb/c-varobj.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdb/c-varobj.c b/gdb/c-varobj.c
index 8fecbd57e08..64c3a9ebf1c 100644
--- a/gdb/c-varobj.c
+++ b/gdb/c-varobj.c
@@ -121,6 +121,7 @@ adjust_value_for_child_access (struct value **value,
       enclosing_type = value_actual_type (*value, 1, &real_type_found);
       if (real_type_found)
 	{
+	  enclosing_type = check_typedef (enclosing_type);
 	  *type = enclosing_type;
 	  *value = value_cast (enclosing_type, *value);
 	}
-- 
2.25.1

[PATCH v2 2/2] Move a comment to a less confusing place

[Christian Walther]

Make it clearer that this comment (from 2024f65) does not apply to the
call to check_typedef further down, which is not on the return value of
get_target_type.
---
 gdb/c-varobj.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/gdb/c-varobj.c b/gdb/c-varobj.c
index 64c3a9ebf1c..acda879bfeb 100644
--- a/gdb/c-varobj.c
+++ b/gdb/c-varobj.c
@@ -86,6 +86,9 @@ adjust_value_for_child_access (struct value **value,
   if ((*type)->code () == TYPE_CODE_PTR)
     {
       struct type *target_type = get_target_type (*type);
+      /* The 'get_target_type' function calls check_typedef on
+	 result, so we can immediately check type code.  No
+	 need to call check_typedef here.  */
       if (target_type->code () == TYPE_CODE_STRUCT
 	  || target_type->code () == TYPE_CODE_UNION)
 	{
@@ -108,10 +111,6 @@ adjust_value_for_child_access (struct value **value,
 	}
     }
 
-  /* The 'get_target_type' function calls check_typedef on
-     result, so we can immediately check type code.  No
-     need to call check_typedef here.  */
-
   /* Access a real type of the value (if necessary and possible).  */
   if (value && *value && lookup_actual_type)
     {
-- 
2.25.1

Re: [PATCH] Fix number of children of varobj with stub debug info

[Christian Walther]

> Given this confusion, I think we should either move to line 89, where you mentioned, or just remove the comments altogether.

Done as a second commit in v2.

> Sounds good, sorry about the run-around. Here's hoping someone with approval permissions looks at your v2

No worries, you made me look at things again in more detail, which is a good thing. Thanks!

 -Christian


-- 
Indel AG
Christian Walther - Software
Tuefiwis 26
CH-8332 Russikon
Switzerland

Tel.: +41 44 956 20 00
www.indel.ch
------------------------------------------------------------------------------------------
New product GIN-MAX4x4: The compact 4-axis motion board now also available as PRO version!
https://www.indel.ch/en/products/drives/compact-motion-drives

[PING][PATCH v2 1/2] Fix number of children of varobj with stub debug info

[Christian Walther]

Ping!

On 2022-06-22 at 17:37, Christian Walther <walther@indel.ch> wrote:
> 
> GDB under some circumstances wrongly reports 'numchild="0"' on a
> variable object for a pointer to a C++ class instance when the type
> description from the debug info is a stub that does not include members.
> The code path is missing a call to check_typedef() to look up the
> complete type description. This commit adds it.
> 
> Additional conditions for the bug to occur:
> 1. "print object" is set to "on"
> 2. The class has no run-time type identification (no vtable).
> 
> Debug information of this kind is generated by Clang 13.
> ---
> 
> Changes in v2:
> - fix code style 
> - add second commit to move confusing comment
> 
> ---
> gdb/c-varobj.c | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/gdb/c-varobj.c b/gdb/c-varobj.c
> index 8fecbd57e08..64c3a9ebf1c 100644
> --- a/gdb/c-varobj.c
> +++ b/gdb/c-varobj.c
> @@ -121,6 +121,7 @@ adjust_value_for_child_access (struct value **value,
>       enclosing_type = value_actual_type (*value, 1, &real_type_found);
>       if (real_type_found)
> 	{
> +	  enclosing_type = check_typedef (enclosing_type);
> 	  *type = enclosing_type;
> 	  *value = value_cast (enclosing_type, *value);
> 	}
> -- 
> 2.25.1


-- 
Indel AG
Christian Walther - Software
Tuefiwis 26
CH-8332 Russikon
Switzerland

Tel.: +41 44 956 20 00
www.indel.ch
------------------------------------------------------------------------------------------
New product GIN-MAX4x4: The compact 4-axis motion board now also available as PRO version!
https://www.indel.ch/en/products/drives/compact-motion-drives