User Details
- User Since
- Dec 6 2017, 1:24 PM (302 w, 5 d)
Fri, Sep 15
Fri, Sep 8
LGTM, thank you Martin!
Wed, Sep 6
I don't see any difference between this patch and the baseline, so that's good news.
Tue, Aug 29
Aug 22 2023
Aug 21 2023
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.
Aug 18 2023
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.
Aug 15 2023
Aug 13 2023
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.
Aug 4 2023
Jun 19 2023
Hello @srj ! Sorry for the late answer.
Jun 16 2023
With double quotes in lit.cfg.py
With binary files.
Jun 15 2023
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.
Jun 14 2023
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.
Jun 13 2023
Thank you for the thorough review @MaskRay !
Thank you for reviewing @alvinhochun @mstorsjo !
Jun 12 2023
Are you able to commit this or can I do it?
Jun 6 2023
Jun 5 2023
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?
May 5 2023
Simplify .drectve section and fix tests for real.
May 4 2023
Re-apply changes & rebase.
May 3 2023
Rebase on D149610
As suggested.
May 2 2023
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.
May 1 2023
Apr 27 2023
LGTM, thanks for the update!
Apr 26 2023
Apr 23 2023
Apr 22 2023
Apr 12 2023
Apr 3 2023
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()?
Apr 2 2023
+ @rriddle
Mar 24 2023
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.
Mar 21 2023
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?
Mar 20 2023
Mar 15 2023
Mar 12 2023
Reid is out for vacations, I’d suggest perhaps to find another reviewer!
Feb 24 2023
Feb 22 2023
Thanks for the changes!
Feb 20 2023
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.
Feb 17 2023
Thanks for the in-depth review @MaskRay, I appreciate!
As suggested by @MaskRay
Feb 8 2023
Feb 5 2023
As suggested by @MaskRay. This changes a few things, but it's for the best I think:
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.
Feb 1 2023
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
Jan 31 2023
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.
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.
Jan 26 2023
Jan 24 2023
As suggested by @arsenm