LGTM, thank you Martin!

In D155579#4636964, @mstorsjo wrote:

How does this behave in your testcase @aganea?

I don't see any difference between this patch and the baseline, so that's good news.

In D157136#4626520, @rnk wrote:

In D157136#4583632, @aganea wrote:

I'm also wondering if instead of this patch, we couldn't craft something simpler, an heuristic that "fixes" the section header and the section symbol. Usually they seems to come in a pair, .text$mn, .text$x or a trio if debug symbols are involved: .text$mn, .debug$S, .text$x. Opinions?

I think there is prior art for doing this for mingw, we do some convoluted stuff to associate .text$_Z3foov / .pdata$_Z3foov / .xdata$_Z3foov together, but IIRC it's not good for performance.

I think for the moment I'm inclined to do nothing, unless this is really high priority for some user. It sounds like we can recommend using llvm-objcopy in place of binutils objcopy as a possible solution.

In D155579#4604541, @mstorsjo wrote:

You who might have more use of nontrivial Windows build scenarios - how much impact would it be to go all in on the new path canonicalization + hash approach, i.e. ditching the file index entirely? Performance wise I would believe that it would be no significant change to before. The only thing that matters probably is how it behaves wrt symlinks/hardlinks, if those are present and in use.

Unfortunately in our case the situation is even worse than what Martin was suggesting. My timings are 2x slower after this patch than without. Tested with a stage2 clang-cl built with ThinLTO and all optimizations on.

Sure. This was meant to catch an issue that only happened on Windows but it should work on all platforms.

Another option could be to add a debug opt to disable this new code path, and skip all the canonicalization and reliability check. It could be one way or the other; either enabled by default or disabled by default. But probably better to enable it by default (this new code path) and let people add the flag if they need more performance.

In D155579#4587472, @mstorsjo wrote:

In D155579#4510035, @thieta wrote:

In D155579#4509952, @mstorsjo wrote:

@thieta IIRC you had a usecase where you scan through large numbers of files with LLVM code somewhere. Are you able to take this for a spin to make sure it doesn't affect the performance of your usecase too much?

Will do, but probably not some time real soon

@thieta Do you have time to try this out in the near future? I'd like to at least know of any potential performance impact before landing this.

Sorry I will answer to your comments a bit later. I was trying to understand the root issue, and I've managed to reproduce it locally.

Hello @srj ! Sorry for the late answer.

In D119049#4429553, @MaskRay wrote:

In D119049#4429521, @aganea wrote:

With binary files.

I see that

config.environment["LLD_SRC_DIR"] = config.lld_src_dir

sets the correct path, but ninja check-lld-unit still fails due to thisPath.append(getenv("LLD_SRC_DIR")); getting an empty string...

With double quotes in lit.cfg.py

With binary files.

In D119049#4429502, @MaskRay wrote:

arc patch D119049
ninja -C /tmp/Rel check-lld-unit

gives me failures.

% grep LLVM_USE_RELATIVE_PATHS_IN_FILES /tmp/Rel/CMakeCache.txt
LLVM_USE_RELATIVE_PATHS_IN_FILES:BOOL=OFF

--
/tmp/Rel/tools/lld/unittests/AsLibELF/./LLDAsLibELFTests --gtest_filter=AsLib.ROCm
--
ld.lld: error: target emulation unknown: -m or at least one .o file required
Failed to link: /home/maskray/llvm/lld/unittests/AsLibELF/Inputs/kernel1.o
/home/maskray/llvm/lld/unittests/AsLibELF/ROCm.cpp:69: Failure
Value of: runLinker("%S/Inputs/kernel1.o")
  Actual: false
Expected: true
ld.lld: error: target emulation unknown: -m or at least one .o file required
Failed to link: /home/maskray/llvm/lld/unittests/AsLibELF/Inputs/kernel2.o
/home/maskray/llvm/lld/unittests/AsLibELF/ROCm.cpp:70: Failure
Value of: runLinker("%S/Inputs/kernel2.o")
  Actual: false

I've reproduced (on Linux) the issue mentioned above. This doesn't happen on Windows/MSVC because __FILE__ (used in the ROCm.cpp test) always expands to absolute paths when building LLVM, since -DLLVM_USE_RELATIVE_PATHS_IN_FILES requires -ffile-prefix-map which is unsupported on MSVC-cl/clang-cl. Instead, I've switched to using an environment variable LLD_SRC_DIR (which stores relative paths where applicable), see changes in ROCm.cpp and lld/test/Unit/lit.cfg.py.

Hello, I’m out this afternoon, is anybody able to disable this test until I can take a look at it please? Otherwise I’ll do it tonight.

In D119049#4418851, @MaskRay wrote:

This will now need a rebase to fix merge conflicts. This is a very desired improvement:)

A reminder that don't forget to test LLD_DEFAULT_LD_LLD_IS_MINGW

Thank you for the thorough review @MaskRay !

Thank you for reviewing @alvinhochun @mstorsjo !

In D86338#4414833, @teemperor wrote:

I do have commit access but this was part of some llvm/Support cleanup that I gave up on a long time ago, so I never merged this. Before merging this someone probably should check that this is still unused within LLVM...

Are you able to commit this or can I do it?

In rGe1aa91b36325086164b2dffd761b6d3960e40171#1212896, @nikic wrote:

Hello @nikic ! When building a two-stage LLVM build, I'm seeing an infinite loop that lead to SimplifyDemandedBits. I thought this might be related to your recent changes. I'm trying to bisect, but I don't have a fast machine, are you able to test on your end?

This is the SimplifyDemandedBits in SDAG rather than InstCombine, so I doubt it is related. Does your build maybe contain https://reviews.llvm.org/D127115? That one is known to cause stage2 hangs and has already been reverted.

Hello @nikic ! When building a two-stage LLVM build, I'm seeing an infinite loop that lead to SimplifyDemandedBits. I thought this might be related to your recent changes. I'm trying to bisect, but I don't have a fast machine, are you able to test on your end?

Simplify .drectve section and fix tests for real.

Re-apply changes & rebase.

Rebase on D149610

In D149610#4315001, @alvinhochun wrote:

In D149610#4312974, @aganea wrote:

@hans @alvinhochun The rationale is to simply remove noise from the output when there are sparse ordinals in the table. I'm not quite sure how useful is to keeps those empty entries, since they will be skipped over by the kernel anyway. [...]

In D149610#4311847, @alvinhochun wrote:

I don't think we need to follow exactly the behaviour of dumpbin. Can you check what Binutils objdump does just for comparison?

objdump skips over empty values, as this patch does (using a test added by D149611):

Fair enough. Hiding completely empty entries may not be so bad then, given that two other tools do the same

The check in COFFObjectFile.cpp, L1567 checks if the RVA is in the output sections. If it's outside the sections, that entry isn't exporting anything, and there will be no dynamic linking at runtime.

Still, I don't quite agree with hiding any entries with non-zero RVA or non-empty names, even if they may not be linked at runtime. My opinion is that if an export entry has either, then it is likely they have been deliberately there for a reason or possibly created in error, so they should still be shown in the output for reference. (I think you can get such symbols using -export:zeroRvaExport=__ImageBase.)

So, I think the condition for skipping should only be RVA == 0 && Name.empty(). I would also consider doing the checks directly in the for loop without adding the ExportDirectoryEntryRef::isEmpty function. Just move move I->getSymbolNameearlier so you can check the RVA and the name before printing the entry.

As suggested.

I'm not quite sure how the parenting works in Phabricator I might be doing things wrong.

Address comments.

@hans @alvinhochun The rationale is to simply remove noise from the output when there are sparse ordinals in the table. I'm not quite sure how useful is to keeps those empty entries, since they will be skipped over by the kernel anyway. The check in COFFObjectFile.cpp, L1567 checks if the RVA is in the output sections. If it's outside the sections, that entry isn't exporting anything, and there will be no dynamic linking at runtime.

LGTM, thanks for the update!

+ @saudi @thieta

Sounds good!
I'm just curious, have you tried experimenting with different sizes? Would it make sense to set a higher value for the mapfile using .SetBufferSize()?

+ @rriddle

LG.

LG.

Just a thought: as future work, we should probably have an integration with LLVM_TOOL_LLVM_DRIVER_BUILD?

Something was missing in the description:

The arguments passed in this option were passed onto the child
process, but we still blindly [used] the clang binary that we had found
to sys::ExecuteAndWait as the intended executable to run.

Fair enough. There are several choices forward: either we mark the issue as "Will Not Fix" or I can try only scoping this patch to only keep the handle open for network drives/paths. Any other suggestions?

Reid is out for vacations, I’d suggest perhaps to find another reviewer!

Thanks for the changes!

As sugested by @MaskRay

@clemenswasser We hit this once in a while, when Microsoft update their libraries. But I'm surprised about the CC at the beginning. How can this issue be reproduced? Can you show a disassembly dump from the VS debugger, around the address of the function that fails?

In addition to these changes, do you think you can add a more descriptive error at a higher level in LLD, maybe in PDBLinker::commit(). It is not clear from the message what the corrective measure is. I suppose adding /pdbpagesize: with a higher block size would fix the problem, but this isn't obvious to everyone.

Thanks for the in-depth review @MaskRay, I appreciate!

As suggested by @MaskRay

As suggested by @MaskRay. This changes a few things, but it's for the best I think:

In D142949#4098579, @MaskRay wrote:

How is auto *ctx = new CommonLinkerContext (with a later cleanup) different from an on-stack COFFLinkerContext ctx; whose destructor is not called?

How is the destructor not called in this patch? ctx on the stack in this patch will call ~CommonLinkerContext() and the would destroy all the instances, the string saver, the bump allocator, etc. We want to avoid that in the "normal" scenario, for a quick exit.

In D115103#4097460, @clemenswasser wrote:

Do you have a suggestion how I could fix this on MSVC

Sorry for the spam. Test coverage would be welcome too :-) The following should exercise the crash: RUN: llvm-readobj --help | head -c1

You'd probably need to remove this #define: https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/Driver.cpp#L1876

In D142949#4094414, @MaskRay wrote:

Do you happen to have more information about which specific line may trigger a problem on Windows? Does this patch regress it?

The previous issue was caused by this call to fatal but you fixed it in rGd9dbf9e30a581fcadd667b6d8e5827a4003b85a2, see problem description in rG45b8a741fbbf.
My whole point is that we don't need this current patch, users can just call lld::safeLldMain().

As mentioned in https://github.com/llvm/llvm-project/issues/59874, this is on purpose. More info here: https://reviews.llvm.org/D142949#4093346. One should call safeLldMain() instead! Let me finish/land D119049 and things should look better afterwards.

In D142949#4093346, @aganea wrote:

The reason for not allocating ctx on the stack is to survive calls to lld::error() in LLD-as-lib scenarios.

Just another quick note -- in theory we can survive crashes as well here. But there's at least a case in lldELF which throws an error() inside a constructor, and that corrupts the heap on ctx deletion, on Windows at least, see rG45b8a741fbbf271e0fb71294cb7cdce3ad4b9bf3

The OP in PR59874 could use safeLldMain instead of lld::elf::link and that will solve their issue (but lld.cpp.obj has to be linked in). With D119049 we should probably decide if we want to expose the naked lld::*::link functions outside of LLD, or if users should always use safeLldMain, or something else.

Thanks for reviewing @jerryyin !
Before going further, I'd like an review/approval from the regular lld contributors as well, whenever you got the chance @mstorsjo @MaskRay @hans @thieta

As suggested by @arsenm