This is an archive of the discontinued LLVM Phabricator instance.

[ASan] Added stack safety support in address sanitizer.
ClosedPublic

Authored by kstoimenov on Oct 19 2021, 3:12 PM.

Details

Summary

Added and implemented -asan-use-stack-safety flag, which control if ASan would use the Stack Safety results to emit less code for operations which are marked as 'safe' by the static analysis.

Diff Detail

Event Timeline

kstoimenov created this revision.Oct 19 2021, 3:12 PM
kstoimenov requested review of this revision.Oct 19 2021, 3:12 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript

Fixed the test to pass.

kstoimenov added a subscriber: kda.
fmayer added inline comments.Oct 19 2021, 5:17 PM
llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
1509–1512

Doesn't need nested if.

I would order this as ClUseStackSafety && SSI && findAllocaForValue(Ptr) && ... (cheapest first).

vitalybuka added inline comments.Oct 19 2021, 6:13 PM
clang/include/clang/Basic/CodeGenOptions.def
227 ↗(On Diff #380829)

We should avoid this flag.
this is internal optimization. if it works we need to make it on
for experiments -mllvm is enough.

Also clang stuff better to extract to a separate patch.

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
815

functionality of the pass should be the same, so I don't see why this is the fatal error. so just ignore it?

btw, why don't you want to support it?

1268

You can make this module pass to calculate StackSafetyGlobalAnalysis and use cached one below.
This way we will avoid exposure of this logic to PM

fmayer added inline comments.Oct 20 2021, 1:52 PM
clang/test/CodeGen/asan-stack-safety-analysis.c
1 ↗(On Diff #380829)

Should this file be in llvm/test/Instrumentation/ instead? Also consider porting some of the tests from HWASan (https://github.com/llvm/llvm-project/blob/main/llvm/test/Instrumentation/HWAddressSanitizer/stack-safety-analysis.ll).

fmayer added inline comments.Oct 20 2021, 1:55 PM
clang/test/CodeGen/asan-stack-safety-analysis.c
1 ↗(On Diff #380829)

Uhm nevermind my first sentence, sorry about that. This is of course the right location.

Removed the top level flag and addressed comments.

kstoimenov edited the summary of this revision. (Show Details)Oct 21 2021, 12:46 PM

I think you also want to use SSI->isSafe(AllocaInst*) in isInterestingAlloca to prevent use-after-scope instrumentation if all accesses are safe.

kstoimenov marked 4 inline comments as done.Oct 21 2021, 12:53 PM
kstoimenov added inline comments.
clang/test/CodeGen/asan-stack-safety-analysis.c
1 ↗(On Diff #380829)

I've removed this file. Also added a .ll test for the internal flag.

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
815

I would like to support it, but I am not sure how to get an instance of StackSafetyGlobalAnalysis, because in the legacy pass manger I don't have access to AnalysisManager<Function> &AM. If you know how to make this work, please let me know.

As far as the report_fatal_error my logic is that if someone sets ClUseStackSafety flag they expect it to work and if it silently doesn't it is a bad user experience.

fmayer added inline comments.Oct 21 2021, 12:55 PM
llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
815

Maybe it would help to look at the HWASan instrumentation pass, which uses the stack safety analysis for both old and new pass managers.

Added support for legacy pass manager and a test for it.

kstoimenov added inline comments.Oct 21 2021, 2:27 PM
llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
815

Thanks for the hint. I've used the HWAsan approach.

This should be good for re-review now. PTAL.

vitalybuka added inline comments.Oct 21 2021, 11:17 PM
llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
816

usually we don't use {} for one liners.
But I'd prefer here:
const StackSafetyGlobalInfo *SSI = ClUseStackSafety ? get...: nullptr

1268

It should probably be in ModuleAddressSanitizerPass, so it wll return ::all() and we don't care about invalidation.

1350

INITIALIZE_PASS_DEPENDENCY(StackSafetyGlobalInfoWrapperPass)

kstoimenov marked 2 inline comments as done.

Fixed test.

Added a comment.

Removed {} after if.

vitalybuka accepted this revision.Oct 26 2021, 4:46 PM

Can you please fix in a separate patch llvm::runPassPipeline: it runs ModulePass after the function pass for -passes=asan-pipeline

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
1509
1528

using poiter after the cast looks cleaner to me
"ignoreAccess(LI, LI->"

This revision is now accepted and ready to land.Oct 26 2021, 4:46 PM
kstoimenov marked 2 inline comments as done.Oct 26 2021, 4:47 PM

PTAL.

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
1268

getCachedResult uses the invalidation so I couldn't get rid of it.

vitalybuka requested changes to this revision.Oct 26 2021, 4:50 PM
vitalybuka added inline comments.
llvm/include/llvm/Analysis/StackSafetyAnalysis.h
93

we still can't do that, some passes can make results irrelevant

This revision now requires changes to proceed.Oct 26 2021, 4:50 PM
kstoimenov marked an inline comment as done.Oct 26 2021, 4:52 PM
vitalybuka added inline comments.
llvm/include/llvm/Analysis/StackSafetyAnalysis.h
93

Looks like only immutable analysis can be used through proxy, and this analysis cannot be immutable.
Maybe we have to convert Asan into ModulePass, like HWAsan.
Any other ideas? @eugenis @pcc @morehouse

eugenis added inline comments.Oct 27 2021, 5:34 PM
llvm/include/llvm/Analysis/StackSafetyAnalysis.h
93

Is the problem here that an invalidated analysis needs to be rerun, and the module-to-function proxy does not know how to do that?

TBH, I'm not sure how big the performance benefits of a function pass are. With the amount of time we've sunk in working around function pass requirements and I'd say just go for it.

kstoimenov added inline comments.
llvm/include/llvm/Analysis/StackSafetyAnalysis.h
93

See https://reviews.llvm.org/D112732. I changed it for the new pass manager only.

kstoimenov updated this revision to Diff 384583.Nov 3 2021, 2:22 PM

After removing function pass.

kstoimenov updated this revision to Diff 384587.Nov 3 2021, 2:32 PM

Restored StackSafetyAnalysis.h.

vitalybuka accepted this revision.Nov 3 2021, 5:39 PM
vitalybuka added inline comments.
llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
1509

check is not needed, we SSGI should be already null in this case

1509–1512

findAllocaForValue is DFS (expensive), stackAccessIsSafe is hash table lookup (cheep)
so I would recommend to switch them

This revision is now accepted and ready to land.Nov 3 2021, 5:39 PM
kstoimenov updated this revision to Diff 384877.Nov 4 2021, 3:01 PM
kstoimenov marked 2 inline comments as done.

Fixed tests.

kstoimenov requested review of this revision.Nov 4 2021, 3:02 PM
vitalybuka accepted this revision.Nov 4 2021, 4:14 PM
vitalybuka added inline comments.
llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
1325

There is ::ProcessedAllocas which will be unnececary overpopulated with irrelevand allocas
Maybe add FunctionSanitizer.Reset()

But please do that in a separate patch, this optimization is unrelated to SSGI

This revision is now accepted and ready to land.Nov 4 2021, 4:14 PM
kstoimenov updated this revision to Diff 384902.Nov 4 2021, 4:26 PM

Moved AddressSanitizer back to the loop.

This revision was landed with ongoing or failed builds.Nov 4 2021, 5:23 PM
This revision was automatically updated to reflect the committed changes.
kstoimenov marked an inline comment as done.
RKSimon added inline comments.
llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
1533

@kstoimenov You're using the LI pointer for all IgnoreAccess calls which is causing nullptr dereference warnings in static analyzer.

Should we just be using I or the dyn_cast<> pointers in each case?

https://llvm.org/reports/scan-build/report-AddressSanitizer.cpp-ignoreAccess-21-f37ec0.html#EndPath

RKSimon added inline comments.Jan 30 2022, 2:50 AM
llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
1533

@kstoimenov Have you been able to check this at all please?

Sorry i just noticed this email. I will take a look next week.

dexonsmith added a subscriber: dexonsmith.
dexonsmith removed a subscriber: dexonsmith.