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.

In D57221#1384651, @mkazantsev wrote:

Aside from refactoring that we can make as follow-up if needed (yet I don't really think it does), does it look OK? :)

Thanks!

The fix makes sense to me but please let others have an opinion too.

We're having issues with this as well, we don't have any libunwind so this change causes
/usr/bin/ld: cannot find -lunwind

I don't know this code at all.

It solves the problem I saw, thanks!

In D57149#1369777, @rnk wrote:

An alternative way to handle this would be to remove unreachable code. I'm surprised unreachable code reaches this pass in a normal pipeline. I thought CGP removed it in normal operation, even at O0. However, removing unreachable code might invalidate the domtree (unless our utility for that updates it?).

Fixed typo, reformulated a piece of a comment.

Not sure who should review this, I added the three last persons that I think did non-NFC changes to StackProtector.cpp.

Is this a proper way of solving the problem? Is it ok to require DT?

In D57018#1366287, @serge-sans-paille wrote:

@uabelho top-of-tree should be fine now.

Just saw build-bots failed the same way

We've been having issues with std::is_trivially_copyable too, and it seems not even this patch solves the problems for us.

The problem I saw goes away with this patch and I haven't seen any new failures with the limited testing I've done on the patch. Thanks!
I don't know this code though, so I don't know if this is the way the problem should be solved.

Great, thanks!

If I change what I think is two typos, then the patch compiles for me and solves the issue I had.

Hi,
A late question about this change. I notice that this change sometimes gives me additional DIFiles in the clang output compared to before.
E.g. if I have a file

I don't know this code but at least it makes the reproducer I had compile succesfully now. Thanks!

I've verifed that it solves the problem I reported. I've done a little bit of testing with the patch and didn't see anything obviously broken yet at least.

I submitted this for markus since he doesn't have commit access yet.

I've got a post-review comment about the use of SCHED_IDLE vs the needed gcc version.

In D53370#1267950, @efriedma wrote:

LGTM.

It looks like you didn't touch LibCallSimplifier::optimizeLog, but I guess that's not necessary (it probably shouldn't be using emitUnaryFloatFnCall anyway).

Was it something like this you had in mind Eli?

Hi,
A late comment about a problem I've noticed with this change.

We are using the Scalarizer in our out-of-tree target and I've run some tests with the patch without problems so I think it's ok.
Please wait a day before submitting though in case someone who really knows this code objects, but if not I think it's ok to push.

In D52540#1247832, @sheredom wrote:

Should I incorporate the test case from the bugzilla into this commit? Given that its a slightly different failure case that is also fixed by my change, seems like its worth ensuring it doesn't accidentally regress again.

Ping?