This is an archive of the discontinued LLVM Phabricator instance.

[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 updated this revision to Diff 357544.Jul 9 2021, 9:32 AM

Formatting again.

fmayer edited the summary of this revision. (Show Details)Jul 12 2021, 3:02 AM
fmayer updated this revision to Diff 357881.Jul 12 2021, 3:14 AM
fmayer edited the summary of this revision. (Show Details)

Formatting.

fmayer published this revision for review.Jul 12 2021, 3:17 AM
fmayer added a reviewer: eugenis.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 12 2021, 3:17 AM

The analysis should not run at -O0.
Otherwise, LGTM.
What kind of code size improvement do you see?

vitalybuka added inline comments.Jul 14 2021, 12:33 AM
clang/test/CodeGen/hwasan-stack-safety-analysis-asm.c
5

this patch mostly change code under llvm/ so tests should be also there, as IR tests

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

Why not from M.getTargetTriple() ?

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

Could you please extract these function in a separate patch?
LLVM likes small close to NFC patches.

400

why we need to check TargetTriple for that?

1285

Instead of // clang-format off
could you replace this with

# comment1
if (...)
  return false;

# comment2
if (...)
  return false;
...

in a separate patch?

fmayer updated this revision to Diff 358564.Jul 14 2021, 5:00 AM
fmayer marked 2 inline comments as done.

Address some comments.

fmayer marked an inline comment as done and an inline comment as not done.Jul 14 2021, 6:19 AM

Addressed inline comments.

clang/test/CodeGen/hwasan-stack-safety-analysis-asm.c
5

I don't have strong feelings, but clang/test/CodeGen/lifetime-sanitizer.c is a very similar test, so I think we should either move all of these to llvm/ or add the new ones here to clang/. What do you think?

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

Mostly for consistency with the legacy pass. Either way is fine for me though, what do you prefer?

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

Because we only need the stack safety analysis if we instrument the stack, which we do not do on x86_64 (see shouldInstrumentStack).

1285

OK, will do in a followup (or see how I can get clang-tidy to do the right thing here).

fmayer updated this revision to Diff 358579.Jul 14 2021, 6:20 AM
fmayer marked an inline comment as done.

rebase

Other than missing llvm test is LGTM

clang/test/CodeGen/hwasan-stack-safety-analysis-asm.c
5

That lifetime tests how clang inserts lifetime markers. So it must be in clang/ this is from https://reviews.llvm.org/D20759 which is clang only patch.
Here the only change for clang is forwarded BuilderWrapper.getTargetTriple().
I don't mind if you keep your tests here, but we also need something which tests llvm without clang as you change llvm tranformation.
Usually if contributor changes code in llvm/, expectation is that check-llvm should discover regression. It's not always possible, but that's the goal and easy to do with this patch.

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

I don't know if will cause any issues, but usually most passes get triple from the module.
I prefer we stay consistent with the rest of the code if possible.

37

!IsOptNull -> Optimize
or
IsOptNull -> DisableOptimization

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

I see, I forgot about this limitation.
LGTM, but unconditional is fine as well, assuming we are going to have stack instrumentation at some point?

440

we usually don't use {} for single line
also maybe good candidate for ?: operator

fmayer updated this revision to Diff 358884.Jul 15 2021, 2:55 AM
fmayer marked 7 inline comments as done.

address comments

Thanks!

clang/test/CodeGen/hwasan-stack-safety-analysis-asm.c
5

Thanks for the explanation. Moved to llvm/.

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

I'll leave it as is, for consistency within this file, as we need to do it this way for the new pass manager.

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

I am not sure we will ever add it for non-LAM x86_64.

440

I think this would make a bit of a long tenary expression.

eugenis added inline comments.Jul 15 2021, 12:32 PM
llvm/test/CodeGen/hwasan-stack-safety-analysis-asm.c
1 ↗(On Diff #358885)

You can't use %clang in LLVM tests. There may be no clang. Use opt to invoke the hwasan pass in isolation, see the tests under test/Instrumentation/HWAddressSanitizer/.

The reason I did not say anything about the tests was because only in clang we can test this thing end-to-end.
Perhaps an LLVM test for instrumentation + a clang test for the passmanager (use -fdebug-pass-manager and -mllvm -debug-pass=Structure for new/old).

vitalybuka added inline comments.Jul 15 2021, 3:35 PM
llvm/test/CodeGen/hwasan-stack-safety-analysis-asm.c
8 ↗(On Diff #358885)

these tests do not work because %clang is not defined here, in LLVM
you can keep them but they need to stay in clang/

my request was to add new tests in llvm-project/llvm/test/Instrumentation/HWAddressSanitizer/
and they need to be *.ll tests
you probably can generate them from C and cleanup manually
or close existing *.ll tests and CHECK for expected difference if analysis is enabled

Also try to generated CHECK statements with llvm/utils/update_test_checks.py, often result is quite useful.

Remove triple

vitalybuka added inline comments.Jul 15 2021, 3:53 PM
llvm/include/llvm/Transforms/Instrumentation/HWAddressSanitizer.h
30

That's what I don't like, passing Triple from BackendUtil.cpp
I've updated the patch. You can download it with "arc patch D105703"

Please create *.ll tests, the rest is LGTM

fmayer added inline comments.Jul 16 2021, 1:43 AM
llvm/include/llvm/Transforms/Instrumentation/HWAddressSanitizer.h
30

OK, no strong feelings but now we addRequire the pass even if we don't need it, so there isn't much point turning it off anymore, no?

fmayer updated this revision to Diff 359326.Jul 16 2021, 8:02 AM

add llvm test

fmayer marked 3 inline comments as done.Jul 16 2021, 8:04 AM
fmayer added inline comments.
llvm/test/CodeGen/hwasan-stack-safety-analysis-asm.c
1 ↗(On Diff #358885)

added an extra llvm test to check from IR, and left the existing clang end to end tests..

8 ↗(On Diff #358885)

added llvm test.

eugenis added inline comments.Jul 16 2021, 9:40 AM
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
408

No need to pass this down, just look at OptimizeNone function attribute.

fmayer marked 2 inline comments as done.Jul 16 2021, 9:54 AM
fmayer added inline comments.
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
408

Interesting, the Aarch64StackTagging code does pass this down, do you know why?

fmayer added inline comments.Jul 16 2021, 9:55 AM
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
408

Also we really should be using this at least in the getAnalysisUsage, which Vitaly's change made unconditional. Correct?

fmayer updated this revision to Diff 359373.Jul 16 2021, 9:57 AM

don't use stack analysis for opt null

eugenis accepted this revision.Jul 16 2021, 10:08 AM

LGTM

clang/lib/CodeGen/BackendUtil.cpp
1174

DisableOptimization=

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

Ah right. Legacy pass needs to declare its analysis dependencies in advance.

This revision is now accepted and ready to land.Jul 16 2021, 10:08 AM
fmayer updated this revision to Diff 359376.Jul 16 2021, 10:12 AM
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
402

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
402

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
406

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
406

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
406

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
403

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

406

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
415

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