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

[Bruno Larsen <blarsen@redhat.com>]

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
>