[PATCH] D13038: dsymutil: Resolve forward decls for types defined in clang modules.

[Frederic Riss via llvm-commits]

friss added inline comments.

================
Comment at: tools/dsymutil/DwarfLinker.cpp:1667-1672
@@ -1656,7 +1666,8 @@
+  
   // FIXME: dsymutil-classic compat won't unique the same type
   // presented once as a struct and once as a class. Use the Tag in
   // the fully qualified name hash to get the same effect.
   // We hash NameRef, which is the mangled name, in order to get most
   // overloaded functions resolvec correctly.
   unsigned Hash = hash_combine(Context.getQualifiedNameHash(), Tag, NameRef);
 
----------------
>> The fact that we hash the Tag here prevents us from uniquing what's in a module and standard namespaces.
> 
> Even if we wouldn't hash the Tag, we would still hash the module's name (NameRef) which is crucial in order to support (Obj)C because it is legal to have conflicting definitions for the same type in two differently named modules.

Yes, I know that. I'm thinking of something different. Let's take a C++ program that uses a module called Foo. If the same program has a namespace called Foo (unrelated to the module), then we could have ODR uniquing collisions between unrelated things if we didn't hash the Tag.

I'm just saying that we should promote the FIXME comment from a bug I wanted to remove to a feature that's needed.





================
Comment at: tools/dsymutil/DwarfLinker.cpp:1750-1751
@@ -1738,9 +1749,4 @@
 }
 
-/// \brief Recursive helper to gather the child->parent relationships in the
-/// original compile unit.
-static void gatherDIEParents(const DWARFDebugInfoEntryMinimal *DIE,
-                             unsigned ParentIdx, CompileUnit &CU,
-                             DeclContext *CurrentDeclContext,
-                             NonRelocatableStringpool &StringPool,
-                             DeclContextTree &Contexts) {
+/// Recursive helper to build the global DeclContext information and
+/// gather the child->parent relationships in the original compile unit.
----------------
> I tried analyzeContextInfo(). We can still rename it if we can come up with something better.
> 

Sounds good.

================
Comment at: tools/dsymutil/DwarfLinker.cpp:1796-1801
@@ +1795,8 @@
+
+  // Prune this DIE if it is either a forward declaration inside a
+  // DW_TAG_module or a DW_TAG_module that contains nothing but
+  // forward declarations.
+  Info.Prune &= (DIE->getTag() == dwarf::DW_TAG_module) ||
+                DIE->getAttributeValueAsUnsignedConstant(
+                    &CU.getOrigUnit(), dwarf::DW_AT_declaration, 0);
+
----------------
> It will prune all DW_TAG_modules that have no children or whose children were pruned.
> 

Sorry I misread the code. Can you add a comment to the function explaining the meaning of the return value please?


http://reviews.llvm.org/D13038