In D80255#2047795, @christylee wrote:

I tried building a more complex workload and the build failed due to some undefined hidden symbol. For thinlto, is the linker or codegen supposed to handle comdat folding?

LGTM

LGTM

LGTM

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.

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.

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".

LGTM

LGTM

Much better! Thank you for making the changes, this patch looks pretty good now. LGTM

In D77672#1968587, @jrtc27 wrote:

In D77672#1968560, @ruiu wrote:

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?

For what it's worth, I have access to various SPARC machines, so can test things as needed. Also, the GCC compile farm (https://cfarm.tetaneutral.net/machines/list/) has multiple SPARC machines; in particular, gcc202 is managed by us Debian sparc64 porters, and has a beefy 64 cores and 64 GiB on a T5 (which, unlike some of the earlier UltraSparcs, has decent single-threaded performance too), so it should be easy for maintainers to test things if they're interested enough. The compile farm is a great resource in general that is really easy to sign up for and most don't know about.

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?

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!

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.

LGTM

LGTM

LGTM

What do you mean by "upstream"?

In D77087#1953957, @int3 wrote:

Hm, I was just trying to emulate ld64's behavior, but I agree that this one might not be very useful. Regardless of whether we throw an error, though, having special-case code to emit it at test time seems useful, if only so we can run the resulting executables on macOS.

In D77086#1953958, @int3 wrote:

Manual line wrapping is kind of a chore, and since we don't have an autoformatter for these files, we'd end up with lines that are too short after doing that sed operation. Plus I was also considering adding -Z to the base %lld substitution to ensure that we never write tests which accidentally depend on system libraries.

LGTM

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.

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.

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.

Although I understand your concrete example, I'm not sure if this is logically correct. This patch is written based on the following assumption:

In D76252#1928528, @grimar wrote:

In D76252#1928449, @ruiu wrote:

In D76252#1928398, @jhenderson wrote:

In D76252#1926005, @ruiu wrote:

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.

Hi @ruiu,

I'm just doing a quick drive by comment here, since I'm not really doing much LLD work at the moment myself. Why are you opposed to yaml2obj being used to generate test inputs instead of llvm-mc? Surely yaml2obj gives more direct control over what is actually in the input? In llvm-mc you only have relatively limited control over some things like relocations.

It seems to me like yaml2obj is a better instrument to be using for the vast majority of test cases, with only a limited set using llvm-mc as a catch-all test to make sure that we don't miss some odd interaction. Of course, this is all making the assumption that mach-o yaml2obj is straightforward to use (I have a feeling it might not be).

You can write tests with yaml2obj too, but if you take a look at ELF, don't you think that we can write fine-grained test cases with llvm-mc too? I want tests to be realistic input files, and in that respect, test cases written in llvm-mc are better than ones in obj2yaml.

I also found that tests written in llvm-mc are easier to maintain. For example, imagine that you already have a test file containing tests for relocations and you want to add a new type of relocation. Then, you'll have to not only add a new relocation to a yaml file but also add a few extra bytes with some addend at a right bit-wise position to the .text section, which isn't easy to do by hand. So, I guess you'll end up writing a test in assembly, compile it and then convert it to yaml -- but if that's the case, we should write tests in assembly in the first place. On the other hand, I didn't find a test case that is easy to write in yaml but not in assembly. There might be some, but that shouldn't be too many. So it looks to me that llvm-mc is a better choice to write test files.

I think there is no universal approach and I agree with both of you. For example, for testing relocation relaxations I'd definetely prefer using YAML because it is important to get the particular op-codes and relocations in the inputs.
It is probaly easier to use YAMLs rather than using llvm-mc and verifying inputs with objdump or alike. For many other things, using llvm-mc is probably a bit more natural.

What I do not agree with is that commiting binaries is OK. From my one last experience after working on
yaml2obj and converting LLVM binaries, I found them are source of all kind of problems. If we can use yaml2obj for that out of box, I think we should.
If we can't then probably it is OK to use binaries temporary, but probably a bug should be reported about lacking features of yaml2obj and the
test should have a FIXME comment with a link.

LGTM

In D76252#1928398, @jhenderson wrote:

In D76252#1926005, @ruiu wrote:

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.

Hi @ruiu,

I'm just doing a quick drive by comment here, since I'm not really doing much LLD work at the moment myself. Why are you opposed to yaml2obj being used to generate test inputs instead of llvm-mc? Surely yaml2obj gives more direct control over what is actually in the input? In llvm-mc you only have relatively limited control over some things like relocations.

It seems to me like yaml2obj is a better instrument to be using for the vast majority of test cases, with only a limited set using llvm-mc as a catch-all test to make sure that we don't miss some odd interaction. Of course, this is all making the assumption that mach-o yaml2obj is straightforward to use (I have a feeling it might not be).

In D76252#1927200, @MaskRay wrote:

In D76252#1926984, @int3 wrote:

The no-id-dylink.s test needs yaml2obj to generate an invalid dylib, but yeah I can replace the other two with binary blobs

We are trying to avoid binary blob in LLVM binary utilities + lld. @grimar has done lots of work in this area. Can you figure out how much effort it takes to implement relevant features in yaml2obj Mach-O? I hope we can avoid binary blobs early.

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.

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.

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.

LGTM

LGTM

Thank you very much for doing this!

LGTM

LGTM

LGTM

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.

LGTM

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.

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.

LGTM with this change.

LGTM

This seems a pretty straightforward port of the existing backends. Very nice!

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.

LGTM

LGTM

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.

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:

LGTM

@MaskRay Please rebase and upload it with me and other people as reviewers, then I'll LGTM.

LGTM