In D62644#1543279, @fhahn wrote:

It looks like this patch may have caused https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=15236. It would be great if you could have a look.

With r363169 the problem seems to be fixed, so this patch is probably not needed anymore.

@fhahn : I noticed that https://bugs.llvm.org/show_bug.cgi?id=31141 stopped crashing with this patch, do you think it's by accident or can that possibly be closed now?

https://bugs.llvm.org/show_bug.cgi?id=41853

When the assertion fails we have:

(gdb) call F.dump()

With this patch the following starts hitting an assertion:

In D60846#1495721, @dlrobertson wrote:

@uabelho great job coming up with a reproducer and thanks @nikic for reverting! I'll start working on a better solution.

I ran ninja check, but I did not hit any failures. Are there more architectures and/or tools I should enable and check before a commit?

This is as far as I've come:

I'm experiencing problems with this patch as well. I'm not 100% sure what the
problem is yet but I have a suspicion:

Anyone has an opinion about this?

Rebased, fixed a spelling mistake.

Should we/someone push this?

Thanks for fixing!
The original case where I saw the crash compiles successfully with this patch.

I see crashed with this patch. E.g.

In D59936#1452754, @hans wrote:

In D59936#1452733, @uabelho wrote:

When looking at some code changes for my out-of-tree target with this commit I noticed a change that I thought I'd ask if you think
is good or not.

With this change we now seem to sink e.g. calls to @llvm.lifetime.end? It returns void, and thus has 0 uses, and as far as I understand
such calls were not sinked before? Is this on purpose and is that good? In general I guess it's good to do lifetime.end at soon as possible?

(And I suppose there are also a whole bunch of other intrinsics that also return void that can be sinked now, I've no idea if that
can cause problems somewhere?)

It's not on purpose in the sense that I was targeting the lifetime intrinsics, but it's showing my change working as intended.

For the lifetime intrinsics, I don't think this really matters. As you say, it's good to do lifetime.end as soon as possible, so sinking them in general doesn't make sense, but I also don't think this matters much here because they can't be sunk very far, i.e. I don't see how this sinking could actually extend the lifetime.

When looking at some code changes for my out-of-tree target with this commit I noticed a change that I thought I'd ask if you think
is good or not.

I noticed that with this commit I get a whole bunch (~40) of warnings like the below when compiling with gcc 7.4:

[10/16] Building CXX object tools/clang/tools/extra/clang-tidy/utils/CMakeFiles/clangTidyUtils.dir/HeaderGuard.cpp.o
In file included from ../tools/clang/tools/extra/clang-tidy/utils/HeaderGuard.h:12:0,
                 from ../tools/clang/tools/extra/clang-tidy/utils/HeaderGuard.cpp:9:
../tools/clang/tools/extra/clang-tidy/utils/../ClangTidy.h:161:16: warning: 'virtual void clang::tidy::ClangTidyCheck::registerPPCallbacks(const clang::SourceManager&, clang::Preprocessor*, clang::Preprocessor*)' was hidden [-Woverloaded-virtual]
   virtual void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
                ^~~~~~~~~~~~~~~~~~~
In file included from ../tools/clang/tools/extra/clang-tidy/utils/HeaderGuard.cpp:9:0:
../tools/clang/tools/extra/clang-tidy/utils/HeaderGuard.h:35:8: warning:   by 'virtual void clang::tidy::utils::HeaderGuardCheck::registerPPCallbacks(clang::CompilerInstance&)' [-Woverloaded-virtual]
   void registerPPCallbacks(CompilerInstance &Compiler) override;
        ^~~~~~~~~~~~~~~~~~~

Great! I'll update the test and submit in a little bit.

I'll push this for JesperAntonsson in a bit since he's having some environment issues.

Added a few nits about too long lines.

Nice! I ran a new bunch of llvm-stress tests with this patch and now I don't see the crashes anymore.
Please push.

Thanks!

In D57149#1401627, @mattd wrote:

I do see the point of ensuring the sanity of StackProtector, and I'm not opposed to patching that up (as you have done here in this patch); however, I do see benefit in preventing the dead code from reaching the StackProtector in the first place. I'm not sure how others feel.

In D58200#1397672, @uabelho wrote:

Hi!

Nice to see this fix get in because we've seen the need for such a change for our out-of-tree target as well.

However, we've added a FixedRegisters argument to tryAssign and trySplit too.
It was a while since we did that change locally but would it make sense to push that into trunk too?

Ping!

Nice to see this fix get in because we've seen the need for such a change for our out-of-tree target as well.

Thanks for fixing!

In D57149#1387325, @mattd wrote:

Hi @uabelho

The test case in your patch definitely causes a problem, but that's because the -start-before/-stop-after is used. If I run that same test through llc, the UnreachableBlockElim pass kicks-in and drops the dead code.
I took a look at the code in your Janurary 24th comment. I was unable to get the same dead block to surface, but most likely I had the wrong flag set. How did you build the sample in that comment?

Managed to drop the new testcase when I rebased >.<

Is this ok or is it better to go down the CGP route instead? If so, can we really guarantee that all dead code is always removed when we get to StackProtector so we don't end up with this problem anyway?

Rebased.