In D139254#4190508, @hiraditya wrote:

Are there possible optimizations which can be implemented to reduce the code-size impact of this change? I'm wondering if this patch adds the checks conservatively to all instances where stack check could potentially, but not necessarily, be missed/useful?

I took a quick look, Seems we can't reproduce the report for test in https://bugs.chromium.org/p/llvm/issues/detail?id=30 in latest llvm.

! In D139254#4036649, @smeenai wrote:
I'm going on an extended leave soon, and I won't have time to implement it before then. I've asked some coworkers to take a look, but if you have the time to add the option it'd be hugely appreciated :)

In D139254#4036649, @smeenai wrote:

In D139254#4034785, @xiangzhangllvm wrote:

Hi @smeenai , do you have plan to implement it ? If not, I will do it, but may some days later (due to some other jobs in my hands).

I'm going on an extended leave soon, and I won't have time to implement it before then. I've asked some coworkers to take a look, but if you have the time to add the option it'd be hugely appreciated :)

Hi @smeenai , do you have plan to implement it ? If not, I will do it, but may some days later (due to some other jobs in my hands).

In D139254#4029517, @smeenai wrote:

Would there be any objections to adding a flag to restore the previous behavior? This is a fairly hefty size increase for us, and we don't think the additional checking is worth the size increase in our case. We also intend to look into the size increases more to determine how much is from guarding calls to e.g. __cxa_throw, where you will reuse the stack frame, vs. calls to e.g. abort or _Unwind_Resume, where the stack frame should never be reused and checking the stack cookie doesn't seem worthwhile (if my logic is sound, which I'm not 100% sure of).

In D139254#4019890, @LuoYuanke wrote:

In D139254#4019615, @lanza wrote:

This diff is a ~0.5% size regression under -Oz when building with -fstack-protector. Is this reasonably expected? And one that should be expected given that it's considered to be a correctness fix? cc @smeenai

I think the size increasement is expected because it inserts more stack corruption checks. Do you find any unexpected check?

In D139682#3987319, @pengfei wrote:

LGTM.

In D139254#3980660, @LuoYuanke wrote:

Could you add a test case for recursively handling __stack_chk_fail?

Add test stack-protector-recursively.ll to check stack protector auto generated function (__stack_chk_fail).

ping, thanks

In D139254#3974001, @alexfh wrote:

There's a problem in the previous iteration of this patch (https://reviews.llvm.org/D138774). It made clang hang 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

Please ensure this is solved here.

In D139254#3976724, @xiangzhangllvm wrote:

In D139254#3974001, @alexfh wrote:

There's a problem in the previous iteration of this patch (https://reviews.llvm.org/D138774). It made clang hang while compiling the following short snippet reduced from an open-source library (compiled on x86-64, linux):

I check it, the clang still hang, let me check and fix it. Thanks very much!

In D139254#3974001, @alexfh wrote:

There's a problem in the previous iteration of this patch (https://reviews.llvm.org/D138774). It made clang hang while compiling the following short snippet reduced from an open-source library (compiled on x86-64, linux):

Update to https://reviews.llvm.org/D139254

Fix Dominate Tree problems

In D138774#3966421, @fhahn wrote:

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.

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!

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

In D138774#3960170, @lebedev.ri wrote:

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.

In D138774#3958882, @lebedev.ri wrote:

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)

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

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

LGTM， thanks

LGTM, thanks for your contribution！

Auto gen is unneccesary.
Try use CHECK-NOT: "assert message" ?

Yes, We use XFAIL to mark a test as an expected failure. An XFAIL test will be successful if its execution fails, and will be a failure if its execution succeeds.
But now we are not expected failure. it is strange to test a test case by check nothing (only expect it not go to assert). I am not much sure about it.
@dsanders any suggestions ?

Let me accept it first, pls wait a little days to see if other reviewers has different opinions

LGTM (Seems we never directly run debugify twice before)

could you add a small test case here ?

Make sense! LGTM

How about merge the " load + cast" to the cast position not load.
for example generate the tileload for line 95 105 to line 105:

Maybe it is time to re-review the previous way for implement "let share define before use".
e.g if let frond end do same "make sure" is more beautiful.

Hi @RKSimon , I sync with Freddy who also use this tool, we decide to merge the disassembler tests. thanks : )

merge disassembler tests

yes, so the *.s must be split with intel and att.
So let make same way for disassemble.
Let me first move them into AMX directory.