This is an archive of the discontinued LLVM Phabricator instance.

Enhance stack protector
ClosedPublic

Authored by xiangzhangllvm on Nov 27 2022, 11:19 PM.

Details

Summary

Enhance stack protector for calling no return function

I find current stack protector didn't work for calling no return function.
This is an defect for "stack protector", so let's refine it.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptNov 27 2022, 11:19 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
xiangzhangllvm requested review of this revision.Nov 27 2022, 11:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 27 2022, 11:19 PM

I reproduce according to https://buildkite.com/llvm-project/premerge-checks/builds/123514#0184bd1c-b61e-4f3f-b814-f4698f5d7b1b guide

The 2 failed tests

Timed Out Tests (1):
  libFuzzer :: minimize_crash.test
Failed Tests (1):
  LLVM :: Examples/OrcV2Examples/lljit-with-thinlto-summaries.test

are all passed in my local:

PASS: libFuzzer :: minimize_crash.test (80256 of 82179)
PASS: LLVM :: Examples/OrcV2Examples/lljit-with-thinlto-summaries.test (48357 of 82179)
(they don't have noreturn info)

I check the following pull request https://reviews.llvm.org/D138778
it has same
Failed Tests (1):

LLVM :: Examples/OrcV2Examples/lljit-with-thinlto-summaries.test

So this fail is not cause by my patch. (And my local is pass)

This pass operates on IR, so it should have IR-based tests (it should be fine to also have codegen tests, since that is a early-codegen ir based pass)

This pass operates on IR, so it should have IR-based tests (it should be fine to also have codegen tests, since that is a early-codegen ir based pass)

Do you mean we should only check IR? I think choose 1 target (e.g X86) to check the asm code may be better. Thanks for review!

pengfei accepted this revision.Nov 29 2022, 10:00 PM

LGTM. But please wait for one or more days for other reviewers.

This revision is now accepted and ready to land.Nov 29 2022, 10:00 PM

This pass operates on IR, so it should have IR-based tests (it should be fine to also have codegen tests, since that is a early-codegen ir based pass)

Do you mean we should only check IR? I think choose 1 target (e.g X86) to check the asm code may be better. Thanks for review!

Did i not literally answer that question in my original comment?
That being said, i'm not actually able to find any IR-producing tests for this pass, which is worrying.

Did i not literally answer that question in my original comment?
That being said, i'm not actually able to find any IR-producing tests for this pass, which is worrying.

Sorry again, let me make more clear, did your "IR-producing tests" means we should check the IR not asm code ?
(Seems it is very normal to check the last asm code when we update IR pass.)

Did i not literally answer that question in my original comment?
That being said, i'm not actually able to find any IR-producing tests for this pass, which is worrying.

Sorry again, let me make more clear, did your "IR-producing tests" means we should check the IR not asm code ?

Yes.

(Seems it is very normal to check the last asm code when we update IR pass.)

For this pass - apparently. In general - no.

Done. @lebedev.ri, clear now, thanks again for explain.

This revision was landed with ongoing or failed builds.Nov 30 2022, 9:25 PM
This revision was automatically updated to reflect the committed changes.

Hello,

I noticed that if the compiler is built with EXPENSIVE_CHECKS on, the stack-protector-no-return.ll testcase fails with

DominatorTree is different than a freshly computed one!
	Current:
=============================--------------------------------
Inorder Dominator Tree: DFSNumbers invalid: 0 slow queries.
  [1] %entry {4294967295,4294967295} [0]
    [2] %unreachable {4294967295,4294967295} [1]
    [2] %lpad {4294967295,4294967295} [1]
      [3] %invoke.cont {4294967295,4294967295} [2]
        [4] %invoke.cont2 {4294967295,4294967295} [3]
        [4] %SP_return3 {4294967295,4294967295} [3]
        [4] %CallStackCheckFailBlk2 {4294967295,4294967295} [3]
      [3] %lpad1 {4294967295,4294967295} [2]
        [4] %eh.resume {4294967295,4294967295} [3]
          [5] %SP_return6 {4294967295,4294967295} [4]
          [5] %CallStackCheckFailBlk5 {4294967295,4294967295} [4]
        [4] %terminate.lpad {4294967295,4294967295} [3]
          [5] %SP_return9 {4294967295,4294967295} [4]
          [5] %CallStackCheckFailBlk8 {4294967295,4294967295} [4]
    [2] %SP_return {4294967295,4294967295} [1]
    [2] %CallStackCheckFailBlk {4294967295,4294967295} [1]
Roots: %entry 

	Freshly computed tree:
=============================--------------------------------
Inorder Dominator Tree: DFSNumbers invalid: 0 slow queries.
  [1] %entry {4294967295,4294967295} [0]
    [2] %SP_return {4294967295,4294967295} [1]
      [3] %unreachable {4294967295,4294967295} [2]
      [3] %lpad {4294967295,4294967295} [2]
        [4] %invoke.cont {4294967295,4294967295} [3]
          [5] %SP_return3 {4294967295,4294967295} [4]
            [6] %invoke.cont2 {4294967295,4294967295} [5]
          [5] %CallStackCheckFailBlk2 {4294967295,4294967295} [4]
        [4] %lpad1 {4294967295,4294967295} [3]
          [5] %eh.resume {4294967295,4294967295} [4]
            [6] %SP_return6 {4294967295,4294967295} [5]
            [6] %CallStackCheckFailBlk5 {4294967295,4294967295} [5]
          [5] %terminate.lpad {4294967295,4294967295} [4]
            [6] %SP_return9 {4294967295,4294967295} [5]
            [6] %CallStackCheckFailBlk8 {4294967295,4294967295} [5]
    [2] %CallStackCheckFailBlk {4294967295,4294967295} [1]
Roots: %entry

This can also be reproduced on a non-EXPENSIVE_CHECKS build just by adding "-verify-dom-info" to the command line like

llc test/CodeGen/X86/stack-protector-no-return.ll -mtriple=x86_64-unknown-linux-gnu -o - -verify-dom-info

Seems not the patch problem, I find the stack protector is strange, it can't be independently print by -print-after-all, and just show in "Module Verifier".
Let me me take a deep look.
thanks very much!

fhahn added a subscriber: fhahn.Dec 2 2022, 5:00 AM

This is causing some bots with EXPENSIVE_CHECKS enabled to fail so I reverted the patch to get the bots back to green: https://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/24249/testReport/junit/LLVM/CodeGen_X86/stack_protector_no_return_ll/

It looks like the StackProtector pass claims to preserve the DT, but with this patch it probably runs into a case where it is not preserved properly, hence the verification failure.

This is causing some bots with EXPENSIVE_CHECKS enabled to fail so I reverted the patch to get the bots back to green: https://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/24249/testReport/junit/LLVM/CodeGen_X86/stack_protector_no_return_ll/

It looks like the StackProtector pass claims to preserve the DT, but with this patch it probably runs into a case where it is not preserved properly, hence the verification failure.

Yes, after we enhance it, the old logic in stack protector is not enough to update the DT. Let me fix it. Thanks!

llvm/lib/CodeGen/StackProtector.cpp
549–550

Thanks, after we enhance the stack protector, the simple DT update is not enough to cover the DT change.

Fix Dominate Tree problems

alexfh added a subscriber: alexfh.Dec 6 2022, 5:36 AM

There's another problem with this patch. Please ensure it's fixed before recommitting. Clang hangs while compiling the following short snippet reduced from an open-source library (compiled on x86-64, linux):

$ cat q.c
char *texsubexpr(char *expression, char *subexpr) {
  char *texchar();
  if (*expression) return texchar(expression, subexpr);
}

void __stack_chk_fail (void) {
  __builtin_trap ();
}

$ clang -O1  -w -fstack-protector-all   -c q.c  -o q.o