[lldb][DWARF] Remove duplicate DIE type assignment

[ljmf00]

Summary

This assignment is already being done via UpdateSymbolContextScopeForType routine at the end of the type parsing function.

[shafik]

LGTM

[labath]

I don't have any issues with this per se, but you may want to sync up with @zequanwu, as his D115308 tries to delete the second instance.

[ljmf00]

In D115662#3192064, @labath wrote:

I don't have any issues with this per se, but you may want to sync up with @zequanwu, as his D115308 tries to delete the second instance.

Maybe he can reach out here, but how is this related to D115308 ?

[labath]

In D115662#3192651, @ljmf00 wrote:

In D115662#3192064, @labath wrote:

I don't have any issues with this per se, but you may want to sync up with @zequanwu, as his D115308 tries to delete the second instance.

Maybe he can reach out here, but how is this related to D115308 ?

You're right -- it isn't as related as I originally thought -- you're deduplicating the dietotype map, and he's doing it for the type list.

But it still is related at least on a conceptual level, as both patches deal with recording information about the type, and would be good if those things could happen in a central place (and you seem to have picked different centralization points). Maybe there is a reason it has to be that way (I honestly don't know) but it at least seems like to be aware of each other's efforts.

[ljmf00]

In D115662#3192735, @labath wrote:

In D115662#3192651, @ljmf00 wrote:

In D115662#3192064, @labath wrote:

I don't have any issues with this per se, but you may want to sync up with @zequanwu, as his D115308 tries to delete the second instance.

Maybe he can reach out here, but how is this related to D115308 ?

You're right -- it isn't as related as I originally thought -- you're deduplicating the dietotype map, and he's doing it for the type list.

But it still is related at least on a conceptual level, as both patches deal with recording information about the type, and would be good if those things could happen in a central place (and you seem to have picked different centralization points). Maybe there is a reason it has to be that way (I honestly don't know) but it at least seems like to be aware of each other's efforts.

In the meanwhile, D115308 got merged. What are your final thoughts on this? I'm going to rebase and test locally but I'm confident that the merged changes doesn't have side effects here.

[labath]

I don't have any issues with this, if everything still works. I hoped there would be more of a discussion on this (I was hoping I'd maybe learn something from it). That obviously didn't happen, but I don't think it needs to hold this back.

[ljmf00]

In D115662#3211312, @labath wrote:

I don't have any issues with this, if everything still works. I hoped there would be more of a discussion on this (I was hoping I'd maybe learn something from it). That obviously didn't happen, but I don't think it needs to hold this back.

I understand your point and I'm fine with discussing this further since this is not a blocker for me :)

I did this as an improvement as I saw this as a duplicate and apparently more duplicates of this assignment (see here https://github.com/llvm/llvm-project/blob/ed6c757d5c5926a72183cdd15a5b1efc54111db0/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp#L1552 ) while reading the code.

To give more context, I'm in the process of generalizing UpdateSymbolContextScopeForType to be able to use it on other non-Clang language plugins (I'm creating a D lldb plugin), see D115201. Both the TypeList and the DIEToType mapping are/were duplicated on ParseTypeFromClangModule. That said, UpdateSymbolContextScopeForType included both and it is useful to do that there due to generalization, although I also understand that UpdateSymbolContextScopeForType is targeted for different tasks.

I propose to either:

1. change the name of UpdateSymbolContextScopeForType to accommodate both insertions (reverting D115308 and removing both assignments on ParseTypeFromClangModule and elsewhere)
2. create a separate routine to perform both insertions (removing it from UpdateSymbolContextScopeForType and elsewhere)
3. simply delete them from UpdateSymbolContextScopeForType and keep it on ParseTypeFromClangModule and other places.

This patch is actually doing none of that since I didn't find the other duplicate assignments at the time of writing it.

I don't have the full picture of this, but this seems to be code that is needed independent of the DWARFASTParser. Looking at older implementations of language plugins (Go, for example), they copy and pasted UpdateSymbolContextScopeForType in their implementation (see https://github.com/llvm/llvm-project/commit/77198bc79b54267f2ce981c3a6c9c0d6384cac01#diff-0cf0246cf7e5ca0f0d43abe83aff7040786afb07703394799a5122cb6b3a18e7L428 ).

Note: I don't have active guidance on LLDB codebase, so I try to learn from the documentation and the actual codebase by myself. Maybe @zequanwu or @labath have more knowledge of the code and they can share their thoughts on this.

[labath]

I am sorry for the delay. This is quite messy so it took me a while to form an opinion on this. I think option one would be the best choice. Doing it there means the these would get set in a central place (that is still fairly near the place where the type is being constructed). The main thing which makes this confusing is the name, and if we change that to something like FinalizeType, then it would look natural.

As an alternative (though I don't know if this would work), we could make a utility function that constructs a type and does all of these finalization steps. That way it would be impossible to forget to add a type to the list, or to add it twice. (I know that there can be multiple DIEs referring to the same type, so it's fine if some extra DIEToType lines remain, but we shouldn't need to set the symbol context scope, or add to the type list twice).

Would you be interested in trying that out?

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

if (!clang_type && (encoding_data_type == Type::eEncodingIsPointerUID ||
     }
   }
 
   type_sp = std::make_shared<Type>(
       die.GetID(), dwarf, attrs.name, attrs.byte_size, nullptr,
       dwarf->GetUID(attrs.type.Reference()), encoding_data_type, &attrs.decl,
       clang_type, resolve_state, TypePayloadClang(GetOwningClangModule(die)));
 
-  dwarf->GetDIEToType()[die.GetDIE()] = type_sp.get();
   return type_sp;
 }
 
 TypeSP DWARFASTParserClang::ParseEnum(const SymbolContext &sc,
                                       const DWARFDIE &die,
                                       ParsedDWARFTypeAttributes &attrs) {
   Log *log(LogChannelDWARF::GetLogIfAny(DWARF_LOG_TYPE_COMPLETION |
                                         DWARF_LOG_LOOKUPS));