[PATCH] D53234: Don't remove COMDATs when internalizing global objects

[ben via Phabricator via llvm-commits]

bd1976llvm added a comment.

Hi Araon,

I am extremely busy at the moment so I'm sorry if I am slow to reply.

In https://reviews.llvm.org/D53234#1299340, @Aaron1011 wrote:

> > Intuitively, we would not expect comdats in LTO object files as they are not required.
>
> I'm not certain what you mean by 'not required'. My view is that LLVM should always respect COMDATs in the IR (e.g. keep them in place, and respect removal rules),
>  since it can't know the exact reason why they were added in the IR. Leaving the COMDAT in place is always a correct option, but removing the COMDAT might break
>  things that aren't expected it.


My understanding is that by design LTO can do system linker "level" optimizations. The object file/s produced by LTO are not supposed to work correctly with the original input files to the link. Tools that use the produced object files must expect that transformations such as COMDAT removal have been applied.

> 
> 
>> LTO can remove the comdats (similarly, so can the system linker)
> 
> Yes, but it needs to follow the rules set out in the spec:
> 
> In the ELF spec, §12 states that "these groups are included, or these groups are omitted, from the linked object as a unit" <https://docs.oracle.com/cd/E23824_01/html/819-0690/chapter7-26.html>.
> 
>   When the linker removes COMDATS, it ensures that it either removes all of the members, or none of them.
> 
> Right now, LLVM is removing the COMDAT group, but leaving all of its members intact - something that the system linker will never do

I believe this is a misinterpretation of the specification. System linkers decide what to include from the input files and then apply further optimizations such as gc-sections, or removing all debug sections. These further optimizations are completely separate from group resolution - so yes the system linker is perfectly legal to remove some parts of a COMDAT group from the final output. I believe that the bfd linker does not remove individual parts of comdat groups when doing gc-sections but gold does... this is a historical mistake in the bfd linker.

> 
> 
>> Presumably, such globals will contain references to the "associated" global which will also anchor those globals.
> 
> This is not the case for CoverageSanitizer and AddressSanitizer. The globals with !associated metadata are simply global variables.
>  There is a 'usage link' from the instrumented function to the global (since the function writes/reads from the global), but the only link from the global back to the function is the !associated
>  metadata and the COMDAT.

Right, but in this case, if the function is removed then the  !associated metadata will be dangling and should have no effect on the output object file, no?

>> Can you post the output of the bfd linker, and a testcase which reproduces the failure with bfd?
> 
> I've created a self-contained example at https://github.com/Aaron1011/llvm_lto_ir
>  It should fail without my patch, and succeed with it.

Thanks. I will take a look when I can.


https://reviews.llvm.org/D53234