fix: walk down type ref debug info when matching symbols

[louiscaron]

I am encountering an issue when updating the A2L file with addresses from a PDB file. The main error reported is:
Remaining portion "X" of "Y" could not be matched

After inspecting both the A2L and PDB files, I confirmed that the required information is present, so I started investigating a possible fix. I implemented a change in the symbol search walk mechanism, but I am not fully confident about this implementation because it may impact the debug information parsed from the ELF file, where you had previously added the following comment:

/// create a type reference for the element type of an array
/// this is (very rarely) needed to break a circular dependency while loading array types

I was wondering whether the type reference in the ELF debug information has the same semantics as the type reference inferred during PDB parsing.

PS: Even after this change, the tool still reports some errors, although the PDB and A2L files appear to be correct. I will likely push additional changes, but I wanted to check with you first to make sure I am not heading in the wrong direction with this initial fix.

PS2: i have just added a unit test for this case

[DanielT]

Generally I've tried to avoid having extra match arms for the TypeRef cases all over the code. The way to do this is the api get_reference.
For example: let typeinfo = typeinfo.get_reference(&debug_data.types);
This function either resolves to the reference target if typeinfo is a TypeRef, or just returns the input typeinfo if it already is a full type.

This call is missing in the match arm for the array. Rather than your fix, I would prefer to add

  let elementaddr = address + (multi_index as u64 * stride);
+ let arraytype = arraytype.get_reference(&debug_data.types);
  find_membertype(
                    arraytype,

In symbol.rs, line 328

That way find_membertype for the array would always run with an already dereferenced type info.

[DanielT]

by the way - I'm not surprised this bug remained hidden until now. Representing the array type with a reference is very rare for DWARF2 debug data, and I think I only ever saw that case once when I added the code whose comment you quoted above.
Meanwhile PDB represents just about everything as a type reference, but PDB was always a very niche use case for me, so I just didn't notice the problem.

[louiscaron]

here, we are using PDB for the virtual ECUs, thanks for the support. I will make the changes you mentioned.

[louiscaron]

one more question, if I make the change you mentioned, is it going to work with nested TypeRef? should the get_reference eventually recursively walk down the TypeRef?

[DanielT]

data type 86308 is unused in this test case, so it should be removed

[DanielT]

alternatively, perhaps you meant to use the TypeRef here.
It seems to me that referring directly to the structtype means the test does not exercise the case where the array element type is a TypeRef.
It's not clear to me what code path you're trying to exercise?

[DanielT]

Your code change is fine.
The test needs a bit of work.
I saw CI raised a lint error regarding code that you didn't touch - you can ignore that, I'll fix it separately.

[louiscaron]

actually, i can explain. I created the types that are created by the parsing of the PDB file. and for a long time i hesitated between resolving the typeref during the parsing or during the symbol type lookup. in the end i chose the symbol type lookup and it made sense regarding your suggestion. but the pdb parsing still does some kind of typeref dereferencing since the intermediate typeref is not referenced by the array type (which i could not figure out why and this is initially why i thought it should be done during the parsing).
when i wrote the test, i tried to see if the array type should point the first nested typeref which raised the other issue that the resolve type is not recurive hence failed.
so i left the typeref, if you want i can write a different test but i do not know if the pdb parsing should fix the typeref dereferencing (i feel we should because we could lose the intermediate debug information from the typeref (modifiers)).

well you know everything know, but since i am not the owner, i will do what you tell me to.

[DanielT]

My concern with the test is that I'll eventually need to understand what it does and why it does that. At that time I will probably no longer remember the details of this PR.
Experience shows that a test should be so focused that it's purpose is obvious, or the test scenario and the intention of the test should be explained in detail in comments.

I've written unfocused tests myself, and they're a pain - when a bad test breaks it might not be clear if the code or the test should be fixed.

[louiscaron]

I will remove the extra typeref creation from the current pr, and I will make a new PR with a full new test for the typeref recurring.but for this, could you confirm that in the pdb with two nested typeref I should end up with two nested typer f in the parsed debugging data?

[louiscaron]

I followed your advice... maybe too much ;-). I started by creating a minimal test exhibiting the issue (with one typeref), but it was not failing the test because actually the find type works find but returns a typeref as the typeinfo of the result. I figured that this was not really what you wanted (you want an actual type), so i added the test to check the returned type and since i was at it, i modified the test to add a second typeref level (as pdb parsing could do it), and i had to modify the get_reference to iterate as long as it matches typerefs

[DanielT]

Looks good. Thanks for the fix!

[louiscaron]

thanks for you for all the work you've put in the tool