- User Since
- Apr 18 2013, 12:16 AM (363 w, 6 d)
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?
Mon, Apr 6
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!
Sun, Apr 5
Wed, Apr 1
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.
Tue, Mar 31
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.
Mon, Mar 30
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.
Tue, Mar 24
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.
Mon, Mar 23
Although I understand your concrete example, I'm not sure if this is logically correct. This patch is written based on the following assumption:
Wed, Mar 18
Tue, Mar 17
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.
Mon, Mar 16
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.
Fri, Mar 13
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.
Thu, Mar 12
Tue, Mar 10
Thank you very much for doing this!
Mon, Mar 9
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
Dec 19 2019
Dec 18 2019
Dec 17 2019
I think I like sym over s as a function parameter name whose scope is not very narrow, but except that LGTM.
Dec 16 2019
Yeah, I guess that showing that tip line would help users.
Looks good but I want someone from PPC side to take a look at this change.
Dec 15 2019
Dec 13 2019
This behavior seems to make sense but is very subtle. Can you add a comment to explain what this is doing?