Skip to content

[clang][fatlto] Only run sanitzer passes in prelink pipelines #160213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[clang][fatlto] Only run sanitzer passes in prelink pipelines #160213

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion clang/lib/CodeGen/BackendUtil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,12 @@ static void addSanitizers(const Triple &TargetTriple,
const CodeGenOptions &CodeGenOpts,
const LangOptions &LangOpts, PassBuilder &PB) {
auto SanitizersCallback = [&](ModulePassManager &MPM, OptimizationLevel Level,
ThinOrFullLTOPhase) {
ThinOrFullLTOPhase phase) {
Copy link
Contributor

@nikic nikic Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ThinOrFullLTOPhase phase) {
ThinOrFullLTOPhase Phase) {

// FatLTO pipelines already added these to the prelink pipeline.
if (CodeGenOpts.FatLTO &&
(CodeGenOpts.PrepareForThinLTO || CodeGenOpts.PrepareForLTO) &&
ThinOrFullLTOPhase::None != phase)
Copy link
Contributor

@nikic nikic Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... I think this condition is too specific and fragile. Have you checked that this also works for the -flto=thin variant? Because I believe that one is going to use ThinOrFullLTOPhase::ThinLTOPostLink here.

I think the correct way to fix this is to just check for a non-post-link phase here (without any checks for other options) and to adjust

llvm-project/llvm/lib/Passes/PassBuilderPipelines.cpp

Line 1727 in 8c122e5

buildModuleOptimizationPipeline(Level, ThinOrFullLTOPhase::None));
to pass one of the post-link phases.

Copy link
Contributor Author

@ilovepi ilovepi Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... I think this condition is too specific and fragile. Have you checked that this also works for the -flto=thin variant? Because I believe that one is going to use ThinOrFullLTOPhase::ThinLTOPostLink here.

It did test it locally with -flto=thin. I should have a test for those in this patch though.

I think the correct way to fix this is to just check for a non-post-link phase here (without any checks for other options) and to adjust

llvm-project/llvm/lib/Passes/PassBuilderPipelines.cpp

Line 1727 in 8c122e5

buildModuleOptimizationPipeline(Level, ThinOrFullLTOPhase::None));

to pass one of the post-link phases.

So use the postlink phase to finish the compile? We could try that, but 1) I thought you were opposed to it. 2) I'm a little concerned there are things in the post link pipelines that won't generate nice object files for a single TU.

For instance, we drop most of the WPD metadata after the prelink pipelines run, but since the default module optimization doesn't run WPD passes, if anything is missed, it doesn't end up having an effect on the object code under Full LTO. Under ThinLTO we have some remaining issues like #139440, where the prelink pipeline starts modifying the vtables, and the normal module optimization goes awry. There we tried to just go back to cloning the modules, but #139999 introduced other regressions. Using the post link pipeline instead, I think would make both pipelines subtly wrong under WPD, whereas today WPD works fine under FullLTO w/ -fat-lto-objects.

I can give the postlink piplines a try and see how it goes in Fuchsia builds. That should give us a little confidence that its not 100% broken.

Copy link
Contributor

@nikic nikic Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, I don't mean using a different pipeline, just passing a different phase parameter. I think that should barely affect the behavior of buildModuleOptimizationPipeline apart from passing a different phase to the pipeline callbacks.

Copy link
Contributor Author

@ilovepi ilovepi Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the build passed https://ci.chromium.org/ui/p/fuchsia/builders/try.shadow/core.x64-lto/b8702634878448393969/overview. The only failures were 2 known flakes, so I guess its maybe safe enough. I'll update this PR to try that method and see if it also avoids the warning issues correctly. I'll also check the WPD issues aren't showing up in either Thin or Full LTO . I may not get this finished until early next week though.

return;
if (CodeGenOpts.hasSanitizeCoverage()) {
auto SancovOpts = getSancovOptsFromCGOpts(CodeGenOpts);
MPM.addPass(SanitizerCoveragePass(
Expand Down
Loading