[ThinLTO, NewPM] Register sanitizers with OptimizerLastPassBuilderHook

[vitalybuka]

Summary

Clang needs to register sanitizer at OptimizerLastEPCallback.
However those callbacks are executed on the ThinLTOPostLink and we had
no way to configure that.

OptimizerLastPassBuilderHook let clang to configure PassBuilder in ThinLTO backed.

This fixes sanitizers with distributed ThinLTO. Non distributed is not fixed yet.

[vitalybuka]

Test will follow if you agree with approach.

[tejohnson]

clang/lib/CodeGen/BackendUtil.cpp

1574

I can't find where this is defined.

llvm/include/llvm/LTO/Config.h

56

Is this essentially the new PM equivalent to PreCodeGenPassesHook above (since this new hook runs at the end of opt)?
Also, might be good to name this like OptimizerLastPassBuilderHook or something like that to indicate its position within the opt pipeline.

[aeubanks]

This makes sense to me, we need to add the PassBuilder callbacks in both the prelink/normal and postlink steps, and those are separated.

llvm/include/llvm/LTO/Config.h

56

I think the idea is that this callback adds PassBuilder callbacks in various parts of the the pipeline. Perhaps just PassBuilderHook? It's weird that we only allow one, but I guess we can make it a vector in the future if necessary.

llvm/lib/LTO/LTOBackend.cpp

306–307

this should go in runNewPMPasses(), not runNewPMCustomPasses().

[vitalybuka]

update

[vitalybuka]

update

[vitalybuka]

clang/lib/CodeGen/BackendUtil.cpp

1574

to avoid noise I extracted into a separate NFC patch, to avoid, line 1061

llvm/include/llvm/LTO/Config.h

56

Kind of. PreCodeGenPassesHook is codegen and it's Legacy PM only.

56

I guess the point of OptimizerLastPassBuilderHook name is to show that this is not any PassBuilder, but the one used in ::opt()

[vitalybuka]

new line

[tejohnson]

clang/lib/CodeGen/BackendUtil.cpp

1067

Some comments around the cases being checked in this if-then-else would be helpful.

1356

I don't understand what you mean by "O0 called optimized"?
Also maybe make it clear that the first sentence goes with the second check?

Also, it isn't clear to me how this is preventing the sanitizers from being added in the ThinLTO pre-link compiles for optimized compiles, as shown in your tests. Unlike regular LTO pre-link optimized builds, which are still getting the sanitizers.

1573

This will add sanitizers for the ThinLTO post-link compiles in a distributed build environment. Does something need to set these up for ThinLTO post-link compiles in the in-process ThinLTO case? In that case thinBackend() is invoked via LTO.cpp through the linker.

llvm/include/llvm/LTO/Config.h

53

This one should say NewPM only.

55

This one should say legacy PM only

56

My point was just that they are effectively at the same place (the end of opt is pretty much the beginning of codegen), so I was wondering if it was worth making the legacy and new PM hooks be at the same place. But it probably doesn't matter much.

[vitalybuka]

update

[vitalybuka]

clang/lib/CodeGen/BackendUtil.cpp

1356

Reworded:
OptimizerLastEPCallbacks is good for anything that not SkipThinLTOPostLink
We need protection only for OptimizerLastEPCallbacks+O0

1573

Yes. And I don't have good idea how to handle that.
Maybe addSanitizer() needs to be moved into
PassBuilder, but not sure how to get CodeGenOpts and LangOpts there.

For now this patch does not brake that case as it's already broken. Maybe we can resolve this later if needed.

llvm/include/llvm/LTO/Config.h

55

Codegen one is invoked on codegen PM which is different from opt PM
For now codegen PM is always legacy

[vitalybuka]

clang/lib/CodeGen/BackendUtil.cpp

1356

Reworded:
OptimizerLastEPCallbacks is good for anything that not SkipThinLTOPostLink
We need protection only for OptimizerLastEPCallbacks+O0

We need special protection only against (SkipThinLTOPostLink && O0)

[aeubanks]

clang/lib/CodeGen/BackendUtil.cpp

1070–1071

We could fix this. It doesn't make sense for only -O0 to run OptimizerLastEPCallbacks prelink. Would that help simplify this?

[vitalybuka]

clang/lib/CodeGen/BackendUtil.cpp

1070–1071

Do you mean move optimizer of -O0 into PostLink as well?
Yes, this would be more consistent.
Still I guess it should a separate patch.

[tejohnson]

clang/lib/CodeGen/BackendUtil.cpp

1069

nit: s/provideds/provided/
Also the line wrapping is off.

1070–1071

Or does it make sense to go the other way, i.e. find a way to add the sanitizer passes to the end of the pre-link of ThinLTO like for regular LTO? The ThinLTO pre-link doesn't execute the module optimization pipeline, but presumably at the end of buildThinLTOPreLinkDefaultPipeline we could invoke the OptimizerLastEPCallbacks? That avoids the issue of trying to get this working separately for in-process ThinLTO. And would be more consistent with regular LTO.

1079

You should only have to check PrepareForThinLTO to see if we are in the pre-link.

1081

nit: s/do not triggers/does not trigger/
Also the line wrapping is off.

1358

nit: s/allow to instromen module/allow instrumenting the module/

[vitalybuka]

fix language

[vitalybuka]

undo CodeGenOpts.ThinLTOIndexFile.empty()

clang/lib/CodeGen/BackendUtil.cpp

1070–1071

I started from PreLink approach but @aeubanks advised the PostLink. At this point I slightly prefer it over PreLink.

Sanitizers In PostLink pros:
+ instrumentation of optimized code produces smaller binary
+ optimizer called after instrumentation may removes some checks and we miss some bugs (It's the case for non-LTO for at least msan. It stops reporting some bugs. Maybe just a bug in msan.)
+ StackSafetyAnalysis supports ThinLTO and can be used to optimize sanitizers.
+ consistent with non-LTO pipeline
+ OptimizerLastEPCallbacks called once per module (I guess not true for full LTO).

Sanitizers In PreLink pros:
+ in-process ThinLTO. does not need special handling
+ Simpler patch
+ Can keep inconsistent -O0 pipeline (not sure why)
+ consistent with regular LTO (but why regular LTO is not consistent with ThinLTO here?)

WDYT if ThinLTO PreLink with sanitizer use buildO0DefaultPipeline?

1079

It needed for clang/test/CodeGen/cspgo-instrumentation_thinlto.c:26:53

[vitalybuka]

clang/lib/CodeGen/BackendUtil.cpp

1070–1071

Sanitizers In PostLink pros:
+ instrumentation of optimized code produces smaller binary

Strangely it's an opposite. On our "internal large binary" I see smaller Asan/Msan sizes if we instrument PreLink.
So this pro can be discarded.

At this point I don't have strong preference.

How can we agree which approach to use?

[vitalybuka]

rebase

[vitalybuka]

rebase

[tejohnson]

clang/lib/CodeGen/BackendUtil.cpp

1070–1071

I like the approach of doing the instrumentation in the prelink for now. The consistency with O0 and with regular LTO is a bonus, and the ability to handle in-process ThinLTO is a big benefit.

It may have some benefits as you found with the binary size because we can apply ThinLTO optimizations to the instrumented code.

I'm curious - what is the issue with StackSafetyAnalysis when this is done in the pre link?

[vitalybuka]

clang/lib/CodeGen/BackendUtil.cpp

1070–1071

I'm curious - what is the issue with StackSafetyAnalysis when this is done in the pre link?

It's just about efficiency. StackSafetyAnalysis supports ThinLTO and in PostLink it can prove more safe allocas including even cross-module calls. It's good to have, but realistically we may never need this. We don't even yet integrated StackSafetyAnalysis into sanitizers (so far it is used only by SafeStack and MemoryTagging which are codegen passes).

In-process support looks more important.

[vitalybuka]

in favor of D96320