User Details
- User Since
- Apr 18 2013, 12:16 AM (278 w, 3 d)
Today
Fri, Aug 10
- use ParallelForEach to rewrite symbols in object files
- Do not touch BinaryFiles
- Use DenseMap to make it faster
- Do not visit BinaryFiles and BitcodeFiles
LGTM
- removed unrelated space changes
Thu, Aug 9
I believe this code makes sense, but this is honestly a bit complicated (that's not your fault but by the nature of the linker script itself). I'm curious what program you are trying to link with this patch. Or, are you just fixing it because it's there?
LGTM
Wed, Aug 8
LGTM
You don't need to call fetch again to add a file that you already have to the symbol table. You can directly add it by calling SymbolTable::addFile().
LGTM with these changes.
Tue, Aug 7
This code was written when we supported only /debug and /debug:dwarf. At that time handling them as two separate options was a reasonable design. Now, we have /debug and /debug:{full,ghash,dwarf,symtab}, so I believe it's time to handle this just like other options that take a value after colon.
These tests were written in ObjectYAML format since LLVM MC support was very incomplete for RISC-V, and the latest llvm-mc from trunk still cannot assemble these tests correctly due to lack of addend handling. I did include the original assembly text in the tests so once the LLVM port is upstreamed we can just swap it to run assembler proper.
LGTM
Mon, Aug 6
Apologies for my belated response. I think this patch is generally looking good. One thing I'd fix before submitting is to rewrite a test using assembly. We do not usually use YAML file for testing. Is there any reason you needed to write them in YAML?
LGTM
Looking at this a bit more, I see that it is unconditionally setting the DefaultImage base for all platforms, which cannot be right. I didn't notice this when it was committed into FreeBSD, but it doesn't matter there, of course. I'm not sure if there is a good way to override this setting on a per-platform basis. Maybe just if (os == FreeBSD) DefaultImageBase=x?
This flag was added for ifunc support, but I'm not 100% sure if it is needed at all. Does lld always assume a common page size of 4096 (or whatever the OS reports as PAGESIZE)?
Fri, Aug 3
LGTM
Generally looking good.
Is this the right patch? Looks like this is the same as the another one.
In ELF, we took a different approach. What we did in the lld/ELF driver is this:
I guess so. There's no reason to be different, I believe.
--undefined doesn't report an error if an argument symbol is missing, at least for Unix. Is this different in lld/wasm?
Doing it probably makes sense, but at the same time this new behavior is subtle. What is the issue with Emscripten without this patch?
LGTM
If you are using git, you can actually combine this patch work your another patch as a single patch.
Please add this option to the manual.
If you are using git-svn, can you combine this patch with your another patch for lld, so that we can review this API-changing patch as one patch?
No, that's not really the case. I strongly suspect that if you land this, you'll see some weird behavior in the wild.
This is very interesting!
LGTM
Thu, Aug 2
Could you give me a few days to review code since we are working in different time zones?
I think this change is a bit dangerous to land because this patch removes the safeguard to not pull out the same object file more than once from an archive file. If our symbol resolution logic is as simple as you described, and if consistency between archive file index and actual object files in it is maintained for all input files, this change should be fine. But I think taking that risk is unnecessary. (I vaguely remember that there was a some problem if we don't have this code, but I can't remember it now.)
LGTM
LGTM
Wed, Aug 1
LGTM
Overall looking good.
Tue, Jul 31
Overall looking good, but who sets Config->ARMJ1J2BranchEncoding to true? Looks like the flag is currently always false.
- 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.
Ah, I misunderstood that the new code was added after addCombinedLTOObject, but the new code is actually before the call of that function. Now the code makes more sense.
Mon, Jul 30
I know the total number of lines didn't change, but I wanted to double-check that you didn't modify any of these lines. Given all lines remain unchanged, LGTM.
Is this line-move-only change?
I'm wondering if we have to keep the list of symbols that could be added as undefined symbols by LTO. If LTO adds new undefined symbol, we always have to resolve them whatever they are, no?
Please create a small patch like this one next time for ease of code review. Thanks!
I picked up a few tests and double-checked that these tests are correct by using undname.
It might be a silly question, but are these functions (e.g. udivhi3 or mulxf3) are functions in static archives? It seems like calling such small functions have relatively large overhead, which appears contrary to the aim of LTO. I'm wondering why LTO creates new function calls to them. Don't you want to inline these functions using LTO?