Check if LLD is built when checking if lto_supported

[jvoung]

Otherwise, older copies of LLD may not understand the latest bitcode
versions (for example, if we increase ModuleSummaryIndex::BitCodeSummaryVersion)

Related to #90692 (comment)

[jvoung]

@vitalybuka or @MaskRay would you mind reviewing, or is there anyone else you suggest?
Context is #90692
Thanks!

[MaskRay]

Perhaps we should lld-available stricter to mean that we have lld in the build directory.

llvm/utils/lit/lit/llvm/config.py has some use_lld code. Can we reuse it?

[jvoung]

I think that traces back to 2344a72 where there is an "OR" (if (LINKER_IS_LLD OR LLVM_TOOL_LLD_BUILD)...), IIUC

@vitalybuka do you have additional context here or thoughts for this too?

#90692 itself is not a huge thing, but I think it's worth figuring this out so that the next person who increases ModuleSummaryIndex::BitCodeSummaryVersion doesn't run into the same issue.

[vitalybuka]

Without 2344a72 some tests will not be executed as LLD will not be detected.
There are multiple ways to build compilr-rt, and I am not sure which will fail with use_lld.

Also use_lld probably can be used with system LLD, but compiler-rt by default is built with just build clang.

[jvoung]

Thanks for the info Vitaly!

Do you remember any of the example tests that were not being executed but should have been and bots (or configuration) to double check?

Did the tests only want the freshly built LLD, and somehow the LLVM_TOOL_LLD_BUILD part wasn't sufficient? Or did the tests want to allow system LLD (but were not testing LTO)?

I'm not sure whether LLVM_TOOL_LLD_BUILD is sufficiently different from the use_lld in llvm/utils/lit/lit/llvm/config.py. It seems to roughly search:

self.config.name.lower() + "_tools_dir",
            "lld_tools_dir",
            "llvm_tools_dir",

lld_tools_dir doesn't seem to be bound in the compiler-rt, and config.name is <unnamed>, so if we use that it would just be checking llvm_tools_dir IIUC. I'm also assuming the use_installed parameter is false, since we don't want that.
And it sets LD_LIBRARY_PATH, but that hasn't seemed to have been a problem for compiler-rt so far?

====

Otherwise, is there a strong reason to have lld-available imply lto_supported and not decouple? Is the concern that this was already assumed in a lot of places and we'd need to inform people that it's decoupled?

[jvoung]

Hi, just wanted to check if you have another other thoughts on the earlier questions, or on how to proceed here?

Thanks!

[vitalybuka]

Hi, just wanted to check if you have another other thoughts on the earlier questions, or on how to proceed here?

Thanks!

Unfortunately there is not much to add.

These are bots maintained by my team: https://lab.llvm.org/buildbot/#/waterfall?tags=sanitizer

All steps print number of executed test.
https://lab.llvm.org/buildbot/#/builders/269/builds/9539

On some of them 2344a72 enabled additional tests.
My guess it was sanitizer-aarch64-linux or sanitizer-x86_64-linux.

It should be OK to proceed with:

1. Land this patch
2. Compare number of tests on build before and after the patch on those bots.
3. If regressed, revert and investigate

[jvoung]

Thanks Vitaly! Ok I can check the number of test before and after and post them.

Would you mind helping land this if it's ok? I don't yet have commit access.

[vitalybuka]

Sure.
However, please, if it breaks bots, bring to my attention those, email, or even better Discord.
The bots are configured in such a way that you are on the blame list, and they will send messages from the bot to you, but not to me.

[MaskRay]

Thanks. This makes the feature lld stricter. Since we don't have rg 'REQUIRES.*!lld' and rg 'UNSUPPORTED.*lld' tests, this should be safe. (I still don't have 100% confidence level since runtime tests often expose weird behaviors)

[jvoung]

Thanks!
I compared the number of tests on build before and after in the sanitizer bots: https://docs.google.com/spreadsheets/d/1STjhM3EFr3YldkpCN40Lic2Qk2Ga2uBoqINYub5Fjxs/edit?usp=sharing

overall they look same except one case stage2/ubsan check-cxx on bot sanitizer-x86_64-linux-bootstrap-ubsan.

• There the number of excluded, unsupported, skip, etc. are the same
• Only the number of "passed" / total tests is fewer. I believe that's because the run batched this commit with another commit that reverted/deleted some libcxx tests.

[vitalybuka]

Thanks for detailed comparison!
ubsan check-cxx Should not be affected by this patch, libcxx does not use these CMake files.