LGTM

LGTM

LGTM

Adding a new option is a big deal. Could you elaborate on why you want to make these symbols hidden ones?

This seems tricky to me. I wonder if you can just fix the problem by reordering symbol assignments in your linker scripts. Also, by allowing forward reference in an assignment, you could theoretically write circular symbol assignments, e.g. foo=bar and bar=foo. That is logically a bit too complicated to me. Maybe we should just keep the linearity of the symbol assignment, instead of supporting this corner case?

LGTM

LGTM

This seems like a straightfoward change to allow users to discard .dynamic.

LGTM

LGTM

We might be able to change NetBSD's tar but I guess that takes very long time. This test is not very important in the sense that this tests just test a corner case. So I think we should just land this to make it work on NetBSD.

I thought NetBSD's tar is bsdtar because it's BSD... Anyways, I think I'm fine with this change, as the new test (which matches both foo\\.o and foo\.o) does not match a string that we don't want it to match.

Sorry, I'm really confused. Could you explain again for me why you need this patch to link Linux kernel?

Looks like what you are trying to do is to generate a x86-64 executable from i386 object files. That's what we do not expect in lld. With this patch, in theory you can create AArch64 exectuables from x86-64 object files, but that doesn't make sense and likely to crash the linker because we assume that all object files are uniform in terms of target types.

Hmm, you said that you are handling inputs that are mix of i386 and x86-64 object files, so I thought this is for that issue. What am I missing?

Even with this patch, don't you still have to make a change to lld so that it accepts a set of input files of different targets?

I ran lld with a few more large programs, and here is the result.

LGTM

I'd like to see more drastic refactoring of this code. Honestly we should completely rewrite this code because the current code is really hard to read. As to this patch, I'm not sure if this is an improvement from readability perspective, as both the current code and the new code are hard to understand to me.

What is the motivation to do this? We don't try too hard to implement every detail of the linker script because it's just too complicated and there is usually an easy workaround that works without a linker script.

LGTM

It seems to me that just adding --start-lib to his command line can fix the issue, so I'm waiting for Robert's response. If it doesn't work for some reason, we can analyze why it doesn't work and then discuss what we can do for his problem.

LGTM

Yeah, I think with an optimizing compiler you cannot see any difference between the old and the new code.

I will collect more data points even though we cannot share programs with you to be fair, so please wait for a while.

Please add this as a test.

What we should and do actually care about are programs whose size is multi-gibibyte that take 30 seconds to a few minutes to link. Unfortunately I cannot share the programs with you, but we observed both speedup and memory usage reduction with this patch.

As one of the people who cares about chromium most in the lld community, I can say that 16 milliseconds regression of linking chromium without debug info is totally negligible.

I think you should focus on large programs. We don't really care about the marginal improvements or regressions for small programs because linking small program is very fast anyway, and we don't care about 10ms improvements or regressions. For large programs, it seems reasonably positive. Also this patch only deletes code.

LGTM

LGTM

Not needed anymore?

Not needed anymore?

Not needed anymore?

I do not have any further comments. LGTM

But 2GB is perhaps still too big and I guess a large part of it can be for dead sections. If we fix this, I'd like to fix it in a proper way so that we can completely eliminate debug info for dead sections.

LGTM

LGTM

Looks good, but I just want to make sure that you have already run the exact same test with MSVC to get the same result.

LGTM

I really don't like to add more code to the linker script support to make it more and more compatible with GNU, as there is not official language spec and there will always be an incompatibility. How much important is this patch? I think you should discuss that more before start trying to fix. We should not try to "fix" all the reported "issues".

LGTM. Please commit.

LGTM

Yeah, I believe this patch is fine to submit, but since Peter is in a different timezone, I wanted give him a chance to review this one.

I wouldn't rush to submit this now, given that this issue is not new at all. Maybe we can just wait for Peter's response?

Thank you for the patch.

LGTM

Somewhat tangent to this patch, but is 64KiB a reasonable default for -z max-page-size? I believe that max-page-size is set to 64KiB so that OS/CPU whose minimum page size is 64KiB can load an executable linked with lld, but is there any real target OS/CPU that does not allow 4KiB pages? Or, is there any other reason to use that default value?

Overall, this patch needs more comments. lld should be readable to those who knows what the linker is but don't know too much about wasm, so you should explain what event symbol/section/etc. are.

This works, but it may better use a SHF_ALLOC section (not sure if that is what ld.bfd uses)

It feels this patch modifies too many places to fix a cosmetic issue. Doesn't something like this work? https://gist.github.com/rui314/6382995c81689668961caf49a0e695f9

LGTM

Thank you for measuring it. Given these numbers, I wouldn't land this change as StringRef is more straightforward.

I see your point, and I agree with that. But the new code is also error-prone because you can make a mistake to not to add the header size, for example. So not sure which is better.

Hm, the new code doesn't look great as it contains too many repetitions. Do you have any suggestion to make this code better?

I tried this patch myself, but it is hard to say if this patch makes a difference. Probably the performance difference is negligible.

Yeah, but the reason why this was just a sketch is because I believe TargetInfo::getPltEntryOffset no longer make much sense with this patch. Perhaps that should be removed.

In order to make lld's --noinhibit-exec behave like GNU linkers', we need to classify errors into "hard errors" (when that happens we need to stop at a nearest checkpoint) or "soft errors" (when that happens we can continue but the output is not likely to make sense.) We cannot simply continue executing the linker after all error()s until the linker creates an output file because some errors are not continuable, and if we force linker to continue, the linker may crash because of an internal inconsistency. I'm fairly satisfied with the current lld's error handling, so I personally don't think categorizing errors into two groups is worth doing though.

This is a rough sketch but I believe this is towards a right direction: https://gist.github.com/rui314/d919496a6db34483d673f9ebe34d6f89

Then maybe the function is currently at a wrong place?

I believe the memory savings of this patch is not free; it now has to scan the same string more than once to find a null terminator. Even though we want to reduce memory usage of lld for Google, in general, saving memory is not always the goal that we want to achieve. We need to get a right balance. To do that, can you tell me how much memory you can save with this patch, and how much is the performance penalty of this patch?

Yes, but you can teach which is the case.

LGTM

Oh, I misunderstood this patch. Sorry for the confusion. My preference was just what I wrote in the previous comment, but that's not a strong preference or anything, and it also makes sense to make values in a YAML file the same as the ones in the actual file, so this patch is fine to me.