[PATCH] D31735: ThinLTOBitcodeWriter: delete comdats if their keys are renamed

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D31735#719730, @rnk wrote:

> I hadn't considered that ThinLTO might try to import part of a comdat group, splitting the group. That seems like something we shouldn't do.


Note that this isn't ThinLTO importing. This is splitting the module into two pieces (in clang) for the purposes of CFI and devirtualization, where one piece needs to use regular LTO module merging and the other uses ThinLTO style compilation.

In https://reviews.llvm.org/D31735#719738, @pcc wrote:

> Actually now that I think about it, we would still have the asan globals problem for vtables with external linkage. So I suspect that we may have to move all members of externally visible comdats to the merged module if any member needs to be moved. That is orthogonal to this change because we would still need to handle internal comdats, for example by dropping them, as this change is doing.


I'm not familiar with the asan globals problem specifically, but yes, splitting and then removing the comdat would cause an issue if the comdat also contains any values with external linkage.



================
Comment at: lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:79
+        if (ComdatsToDelete.count(C->getName()))
+          GO->setComdat(nullptr);
+        else if (C->getName() != ExportGV.getName())
----------------
pcc wrote:
> inglorion wrote:
> > pcc wrote:
> > > I think it would be simpler to do two passes over global_values, one where you decide which comdats to delete and the other where you delete them.
> > Definitely simpler. Are you ok with iterating all the globals again? This more complex implementation tries to avoid some of the work by immediately deleting references to comdats already slated for deletion and indexing globals by comdat to avoid having to iterate over all the globals again (based on the assumption that most globals either won't have a comdat or the comdat will have the same name as the global).
> > 
> > Also, did you want me to iterate over the symbols in ImportM as well (as per your "remove the comdats from both modules")?
> > Definitely simpler. Are you ok with iterating all the globals again?
> 
> Fine with me, modulo this being the right place for the fix.
> 
> > Also, did you want me to iterate over the symbols in ImportM as well (as per your "remove the comdats from both modules")?
> 
> I think the best place to do it is in a separate function, where this function is responsible for collecting and the other one is responsible for removing. Basically the caller would do something like this:
> ```
>   std::set<Comdat *> ComdatsToRemove;
>   promoteInternals(*MergedM, M, ModuleId, ComdatsToRemove);
>   promoteInternals(M, *MergedM, ModuleId, ComdatsToRemove);
> 
>   removeInternalComdats(M, ComdatsToRemove);
>   removeInternalComdats(*MergedM, ComdatsToRemove);
> ```
Agree with pcc, a two pass solution is cleaner and more readable.


https://reviews.llvm.org/D31735