This is an archive of the discontinued LLVM Phabricator instance.

Object: Improve COFF irsymtab comdat representation.
ClosedPublic

Authored by pcc on Nov 20 2017, 6:39 PM.

Details

Summary

Change the representation of COFF comdats so that a COFF linker
is able to accurately resolve comdats between IR and native object
files. Specifically, apply name mangling to comdat names consistently
with native object files, and do not export comdats with an internal
leader because they do not affect symbol resolution.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc created this revision.Nov 20 2017, 6:39 PM
pcc updated this revision to Diff 123707.Nov 20 2017, 7:26 PM
  • Fix use after free
rnk accepted this revision.Nov 20 2017, 8:22 PM
rnk added inline comments.
llvm/lib/Object/IRSymtab.cpp
160 ↗(On Diff #123707)

Every time I see getNameWithPrefix called without a GlobalValue, I consider it a bit buggy because it won't add the @N byte suffix for __stdcall functions. However, clang always mangles extern "C" names itself for functions with non-standard conventions, so there's no way to exercise this bug from C, it has to come from LLVM IR.

Do we care? Should we fatal error in AsmPrinter when a symbol in a comdat would require a suffix? Most languages that aren't C++ aren't going to care about this.

This revision is now accepted and ready to land.Nov 20 2017, 8:22 PM
rnk requested changes to this revision.Nov 20 2017, 8:23 PM

Didn't mean to push "accept"

This revision now requires changes to proceed.Nov 20 2017, 8:23 PM
pcc added inline comments.Nov 20 2017, 8:48 PM
llvm/lib/Object/IRSymtab.cpp
160 ↗(On Diff #123707)

Thanks. For me that raised the question of how AsmPrinter mangles comdat names. I thought it just called Mangler::getNameByPrefix on the name as I did here, but it looks like in fact we look up the leader symbol using the name of the comdat and mangle its name [1].

That seems really weird to me, but for now it seems best to be consistent with that here. We might consider making a change along the lines you suggest, but that's probably an orthogonal change.

[1] http://llvm-cs.pcc.me.uk/lib/CodeGen/TargetLoweringObjectFileImpl.cpp#1027

pcc updated this revision to Diff 123828.Nov 21 2017, 11:08 AM
pcc edited edge metadata.
pcc marked an inline comment as done.
  • Re-implement Builder::getComdatIndex to be consistent with AsmPrinter and add error handling
rnk added inline comments.Nov 21 2017, 11:31 AM
llvm/test/LTO/Resolution/X86/symtab.ll
61 ↗(On Diff #123828)

We should be able to test it with this IR:

$f = comdat any
define weak_odr x86_fastcallcc i32 @f(i32 inreg %a, i32 inreg %b) comdat {
entry:
  %add = add nsw i32 %b, %a
  ret i32 %add
}

I guess the symtab would say @f@8. =/

pcc updated this revision to Diff 123832.Nov 21 2017, 11:49 AM
pcc marked an inline comment as done.
  • Add test
rnk accepted this revision.Nov 21 2017, 12:53 PM

lgtm

This revision is now accepted and ready to land.Nov 21 2017, 12:53 PM
This revision was automatically updated to reflect the committed changes.