Page MenuHomePhabricator

[hwasan] Use stack safety analysis.
ClosedPublic

Authored by fmayer on Jul 9 2021, 8:05 AM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
fmayer marked 3 inline comments as done.Jul 16 2021, 10:12 AM

fix comment.

Btw Vitaly, will this work with LTO out of the box? I think we used to add pre-LTO StackSafety pass explicitly for memtag only, but it looks like that code is gone.

Btw Vitaly, will this work with LTO out of the box? I think we used to add pre-LTO StackSafety pass explicitly for memtag only, but it looks like that code is gone.

What do you mean gone? Can you show me the patch?

We probably needs something to add analysis for HWASAN as well.

llvm/include/llvm/Transforms/Instrumentation/HWAddressSanitizer.h
30

In case of legacy pass? Correct. I don't think we need to over optimize this deprecated case which is going to be removed at some point anyway.
Let's check DisableOptimization as we have it here anyway, but avoid forwarding Triple.

For new PM it's irrelevant as it's does not declare requirements in advance.

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
427

@eugenis unrelated to the patch, but why do we this args if then we assert(!CompileKernel || Recover);

llvm/test/Instrumentation/HWAddressSanitizer/stack-safety-analysis.ll
6

you don't need to hardcode local path here

11

would be nice to have also function which have __hwasan_generate_tag in either case to make sure
-hwasan-use-stack-safety=1 does not disable tagging completely.

28

usually we remove irrelevant attributes #* and module flags !* to make test simpler

vitalybuka accepted this revision.Jul 16 2021, 12:15 PM

Btw Vitaly, will this work with LTO out of the box? I think we used to add pre-LTO StackSafety pass explicitly for memtag only, but it looks like that code is gone.

What do you mean gone? Can you show me the patch?

We probably needs something to add analysis for HWASAN as well.

https://reviews.llvm.org/D80771

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
427

why not? This forbids CompileKernel && !Recover, how else would you represent the 3 remaining combinations?

vitalybuka added inline comments.Jul 16 2021, 3:15 PM
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
427

why not? This forbids CompileKernel && !Recover, how else would you represent the 3 remaining combinations?

Thanks. I've misread the expression.

fmayer updated this revision to Diff 359714.Jul 19 2021, 3:00 AM
fmayer marked 6 inline comments as done.

improve test.

fmayer updated this revision to Diff 359715.Jul 19 2021, 3:01 AM

style fix

fmayer updated this revision to Diff 359716.Jul 19 2021, 3:01 AM

update

llvm/test/Instrumentation/HWAddressSanitizer/stack-safety-analysis.ll
28

Removed all sorts of unneeded stuff. Thanks.

This revision was landed with ongoing or failed builds.Jul 19 2021, 3:55 AM
This revision was automatically updated to reflect the committed changes.
fmayer reopened this revision.Jul 19 2021, 4:09 AM

Broke some postsubmit bot.

This revision is now accepted and ready to land.Jul 19 2021, 4:09 AM
fmayer updated this revision to Diff 359749.Jul 19 2021, 4:54 AM

remove stack safety asm test.

I removed the stack-safety-analysis-asm.c test because I don't think it really adds anything and it caused problems. SGTY?

eugenis accepted this revision.Jul 19 2021, 1:55 PM

I removed the stack-safety-analysis-asm.c test because I don't think it really adds anything and it caused problems. SGTY?

Absolutely.

LGTM

fmayer updated this revision to Diff 360054.Jul 20 2021, 2:05 AM
fmayer marked an inline comment as done.

format

This revision was landed with ongoing or failed builds.Jul 20 2021, 2:06 AM
This revision was automatically updated to reflect the committed changes.
fmayer reopened this revision.Jul 20 2021, 2:38 AM

Made some buildbot unhappy again. Sorry :(

This revision is now accepted and ready to land.Jul 20 2021, 2:38 AM
fmayer updated this revision to Diff 360076.Jul 20 2021, 3:45 AM

fix required analysis logic

vitalybuka added inline comments.Jul 20 2021, 6:55 PM
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
397

I like this even less than "plumbing", if you need shouldUseStackSafetyAnalysis please just pass Triple as in earlier versions.

Why do you need to avoid loading analysis? Revert does not explain what is broken.

fmayer added inline comments.Jul 21 2021, 2:08 AM
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
397

I am still looking at the breakage. But if you looked at the logic before, there was the case DisableOptimization BUT ClUseStackAnalysis being explicitly set, which made us try to getAnalysis without having required it here.

I also like the plumbing more than this, but changed to this to see what you think.

vitalybuka added inline comments.Jul 21 2021, 1:57 PM
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
401

Why not just:

if (!DisableOptimization || ClUseStackSafety)
  AU.addRequired<StackSafetyGlobalInfoWrapperPass>();
fmayer added inline comments.Jul 21 2021, 2:19 PM
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
401

Because ClUseStackSafety defaults to true, so this will be quite useless except if the user explicitly sets it to false. So then you end up duplicating the logic in shouldUseStackSafetyAnalysis, which is why I went for the Dummy (thinking about it, we don't actually need to setArch on it).

fmayer updated this revision to Diff 360623.Jul 21 2021, 3:31 PM

more flag parsing.

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
401

How about this (mightUseStackSafetyAnalysis)? This seems like a nice tradeoff without duplication of the logic.

vitalybuka accepted this revision.Jul 21 2021, 5:24 PM
vitalybuka added inline comments.
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
398

I think this function without comment is clear enough. But up to you.

401

LGTM

fmayer updated this revision to Diff 360735.Jul 22 2021, 2:09 AM

formatting.

This revision was landed with ongoing or failed builds.Jul 22 2021, 4:05 AM
This revision was automatically updated to reflect the committed changes.
fmayer marked 4 inline comments as done.
fmayer reopened this revision.Jul 22 2021, 4:25 AM

Sorry. broke a buildbot again: https://lab.llvm.org/buildbot/#/builders/139/builds/7613/steps/6/logs/FAIL__Clang__asan_c

I am not sure why I cannot reproduce this locally.

Pass 'HWAddressSanitizer' is not initialized.
Verify if there is a pass dependency cycle.
Required Passes:
clang: /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:715: void llvm::PMTopLevelManager::schedulePass(llvm::Pass*): Assertion `PI && "Expected required passes to be initialized"' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.
This revision is now accepted and ready to land.Jul 22 2021, 4:25 AM
vitalybuka added a comment.EditedJul 22 2021, 9:59 AM

Do you build with assertions enabled (-DLLVM_ENABLE_ASSERTIONS=ON")?

Sorry. broke a buildbot again: https://lab.llvm.org/buildbot/#/builders/139/builds/7613/steps/6/logs/FAIL__Clang__asan_c

I am not sure why I cannot reproduce this locally.

Pass 'HWAddressSanitizer' is not initialized.
Verify if there is a pass dependency cycle.
Required Passes:
clang: /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:715: void llvm::PMTopLevelManager::schedulePass(llvm::Pass*): Assertion `PI && "Expected required passes to be initialized"' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.
vitalybuka added inline comments.Jul 22 2021, 10:04 AM
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
414

To fix that revert you need to add

INITIALIZE_PASS_DEPENDENCY(StackSafetyGlobalInfoWrapperPass)

fix crash

weird, my config with -DLLVM_ENABLE_ASSERTIONS=ON does not reproduce, however exact cmake from bot does

This revision was landed with ongoing or failed builds.Jul 22 2021, 4:20 PM
Closed by commit rG96c63492cb95: [hwasan] Use stack safety analysis. (authored by fmayer, committed by vitalybuka). · Explain Why
This revision was automatically updated to reflect the committed changes.

fix crash

weird, my config with -DLLVM_ENABLE_ASSERTIONS=ON does not reproduce, however exact cmake from bot does

-DLLVM_TARGETS_TO_BUILD=X86 was needed as well