- User Since
- Dec 28 2012, 2:34 PM (293 w, 5 d)
LGTM except for a few spelling/grammar nits.
Please make sure that your patch is formatted to match the LLVM style. clang-format can do that for you.
obj-path should produce the same order as save-temps because they use the same code path -- they both work by setting Filename to a non-empty string, which causes us to follow the code path that appends task identifiers and use non-temporary files. If you're seeing something different, maybe that should be fixed instead?
Doesn't the obj-path option already do what you want here?
Tue, Aug 14
Please add a test with a 32-bit non-PIC executable.
Mon, Aug 13
Do you have commit access?
Fri, Aug 10
Wed, Aug 8
- Add a LazyObject test
Mon, Aug 6
Please upload patches with context. arc diff will do this for you.
The issue is that if two object files depend on one another, we can end up in a situation where SymbolTable::fetchLazy is called recursively multiple times on the same object file, so we need to make sure to do nothing on the second call. Reproducer:
$ cat a.s .globl a, b a: $ cat b.s .globl a, b b: $ as -o a.o a.s $ as -o b.o b.s $ ar cru foo.a a.o b.o $ ld.lld -eb foo.a -m elf_x86_64
With your patch I get:
ld.lld: error: duplicate symbol: b >>> defined at b.o:(.text+0x0) in archive foo.a >>> defined at b.o:(.text+0x0) in archive foo.a
Fri, Aug 3
I don't think the .hidden directive is meaningful in COFF, is it?
Thu, Aug 2
Looks great, thanks.
No, I have comments.
Tue, Jul 31
But if we change the TempFile API so that it always creates the temporary in the same directory as the destination, that becomes a non-problem, no?
Can't we change dsymutil to create its temporary file in the same directory as the destination?
I thought about that but decided against it for a few reasons:
- It seemed best to express what we want to happen directly. Putting the symbols in the object files seemed like a more round-about way of fixing the bug.
- The set of libcall symbols is a property of the code generator linked into lld, not the one that was used to create the bitcode file. That means that if we see an old bitcode file it would not be correct to use its libcall symbols.
- It would bloat every bitcode file with the same (potentially incorrect due to 2) content when that can be easily avoided.
Mon, Jul 30
I'd probably mention in the commit message that it's just the log output that's being made deterministic. As far as I know this shouldn't affect the contents of the output file.
Fri, Jul 27
It is a char pointer.
Thu, Jul 26
I suspect that we aren't gaining much from the flags/relocation count/section size because all of these properties typically depend on the section contents anyway, so I left them out.
- Switch to xxhash
Wed, Jul 25
I'm not sure that DataRefImpl would be a better API because you would need to keep track of a lot of state between getting the info for each PLT entry and that could get complicated. DataRefImpl is better suited for the thin wrappers around object file data structures where you're just enumerating the elements of an array so the between-element state is smaller.
And agreed with eugenis that if you can avoid using the disassembler in findPltEntries that might be simpler.
Looks like we already have https://bugs.llvm.org/show_bug.cgi?id=36272 about this.
I agree with eugenis that the PLT symbolization should live in lib/Object. That would also help us implement PLT symbolization in llvm-objdump. I was imagining that there would be a function that would take an object file and return a list of (PLT entry address, symbol) pairs. That would seem to be sufficient for llvm-objdump as well as for this code.
Tue, Jul 24
I'd prefer if we did something like this rather than changing the linker. I see gcc's non-conformance with the coff spec as a bug and if there aren't any real users of gcc+lld I'd prefer us not to have workarounds for that case.
As I mentioned in https://bugs.llvm.org/show_bug.cgi?id=38281 I don't see a good reason to do this. Also extern_weak is not an appropriate linkage for definitions, only declarations.