- User Since
- Apr 18 2013, 12:16 AM (381 w, 6 d)
May 20 2020
May 13 2020
There might be a better way of doing this. As far as I know, directly referring a non-group-leader symbol is not allowed, so the problem we have here is to find an object file that violates the spec. Do you think you can directly implement that logic? I mean, when a symbol is being resolved, we can check if a symbol is directly referring a group internal symbol, and if that's the case, we can emit an error as soon as such a bad symbol use is found.
May 12 2020
I wonder in what condition this error occurs. In particular, can you trigger this with standard-compliant object files?
Nice. This looks like a straightforward implementation of an archive file support.
Apr 26 2020
I think this behavior was carried over from the previous version of lld, but I believe this would cause confusion than being convenient. This is also against our general policy to make cross-linking easier, which is "lld should not change the behavior on which OS it is running".
Apr 14 2020
Apr 12 2020
Much better! Thank you for making the changes, this patch looks pretty good now. LGTM
Apr 8 2020
Apr 7 2020
Improving SPARC support is fine as long as we don't need to do something special for the ISA. But since no active maintainers have a SPARC machine, it is hard to find bugs in SPARC. Can you add tests for the new relocations?
Apr 6 2020
I had a VC meeting wtih maskray to discuss concerns he has, and he is ok with this patch. I'm fine with this patch too. I'll give a final LGTM to unblock you. Please make the following changes and submit. Thanks!
Apr 5 2020
Apr 1 2020
We usually construct a section in three phases as follows:
The tests for lld/mach-o definitely need to be able to run on non-macOS platforms. We generally run all tests on all kind of bots, so yes, you cannot assume that /usr/lib/libSystem.B.dylib exists. Actually you shouldn't make any assumptions on the existence or absence of any external file.
Mar 31 2020
What do you mean by "upstream"?
I'm not sure why we need this. Could you elaborate a bit? If all executables need to be linked against /usr/lib/libSystem.B.dylib and if a user didn't pass -lSystem, isn't it still their fault and not ours, no?
Overall I'd like to see more comments. All functions and classes that are not trivial should have a comment explaining what they represent or what they are supposed to do.
Mar 30 2020
Instead of --threads=all, can you use --threads? --threads has several advantages over --threads=all:
I'm actually not in favor of this change as I want to keep test files self-contained. I'd stick with lld -flavor darwinnew. Once the new lld/mach-o is ready, you can run sed with s/lld -flavor darwinnew/lld.ld64/g to replace all occurrences of lld -flavor darwinnew. So I don't see a need for abstraction here.
Can you submit this change? I think this is good enough as an initial commit, and you can make further changes on top of this.
Do we need all? It's the default behavior, so if you want to specify --threads=all, you can just delete it altogether.
Mar 24 2020
lld's --gc-sections behavior didn't take start/stop symbols into consideration, but it turned out some programs didn't work with that algorithm, so we (I believe rafael or pcc) added the logic to preserve sections for start/stop symbols. Therefore, I think we can't ignore start/stop symbols at the section gc stage and need to at least keep the existing logic.
Mar 23 2020
Although I understand your concrete example, I'm not sure if this is logically correct. This patch is written based on the following assumption:
Mar 18 2020
Mar 17 2020
As to the tests, we generally don't use yaml2obj to write tests (except for COFF which is historically using yaml2obj). If you are using yaml2obj just until lld itself is able to produce files used by tests, I don't think you have to spend too much time writing tests in yaml2obj. Instead, you can submit binary files for now and then replace them later.
Mar 16 2020
It looks like there's no activities in the llvm-dev thread and this review thread, so I'll reiterate my LGTM, as I think we should not block this change, in particular given that the corresponding change to LLVM has been submitted.
Mar 13 2020
That's discussed on llvm-dev, and I think that even though we should ideally extend
the tool to handle the new bit, it shouldn't block this patch. This patch adds a missing
flag that was added to MSVC link.exe recently.
Jez and Shoaib, if you guys have more patches on this one, you can send them now for review if you want.
Mar 12 2020
Mar 10 2020
Thank you very much for doing this!
Mar 9 2020
Mar 5 2020
Yeah, that's another type of consistency, and if you feel that that consistency is more important, feel free to make a change to wasm-ld so that the tool defaults to runtime OS's rsp-quoting style.
Overall looking good.
I tried to apply your patch to submit, but it looks like your test file (llvm/test/tools/llvm-readobj/COFF/Inputs/has_cet.exe) cannot be downloaded from Phabricator. Can you upload a file somewhere and send me the URL? Or you can encode with base64 and send it to me via email.
Mar 4 2020
But we are going to call the new one as mach_o2 until mach_o can be removed and rename the new one mach_o once it's done, no? If so, I don't think we need to rename only this part at this moment. We can rename them all at once later.
lld's default choices are different among platforms as we are trying to match the native linkers' behaviors. So is true to clang; if you invoke clang as clang-cl (which is a Microsoft-compatible compiler driver), the value is set to Windows by default. I think we didn't have any other policy than this, as we are creating drop-in replacements.
Mar 3 2020
LGTM with this change.
Mar 1 2020
This seems a pretty straightforward port of the existing backends. Very nice!
Feb 27 2020
If Sriraman has already resolved all Fangrui's comments, I can reiterate my LGTM, as the new code is well written, and I don't have a specific concern. I actually think that this is an interesting feature and in some degree a natural extension of -ffunction-sections.
Feb 24 2020
Feb 13 2020
Overall looking good, but it looks like this test file doesn't cover all relocations you want to handle. You are handling the following relocations, and compared to that the test file seems too small.
Feb 12 2020
Feb 11 2020
Feb 7 2020
As we discussed today, this patch didn't make sense to me at first sight, as it redoes a lot of things that we already implemented to lld (e.g. making a new "config" object only for this thing, defining a new type of Symbol (SymbolEntry) even though we have Symbol class, etc.) But the thing is that this code is intended to be used by some external tool. If that's the case, you should consider moving this to LLVM instead and use the code from lld and your tool.
Here is a summary of today's discussion:
Jan 16 2020
Jan 9 2020
@MaskRay Please rebase and upload it with me and other people as reviewers, then I'll LGTM.
Jan 6 2020
Jan 5 2020