Page MenuHomePhabricator

[StackProtector] Don't crash when passing post-SP IR back in
Needs ReviewPublic

Authored by loladiro on May 11 2018, 6:40 PM.

Details

Summary

In julia, we use LLVM as a JIT compiler. A fairly common occurrence
is that we get an LLVM crash assertion in the backend and then extract
the problematic IR module from the core dump. However, this IR already
had the StackProtector pass run on it. Unfortunately, this means
that running llc on it will crash in ISel (because StackProtector
gets confused and assumes that the existence of the prologue means
that we will use SDag-based stack protection, but the necessary
intrinsic prototypes aren't actually defined). This is unfortunate,
because it also prevents using bugpoint on this IR without manually
stripping out the StackProtector artifacts first. This patch aims
to improve that situation by teaching StackProtector to recognize
whether a given prologue means that the SDag based stack protection
mechanism is available. If it is not, this patch does cause the
SP transformation to be applied twice, but that's a lot better
than crashing, because it often still allows the original bug
we're investigating to be reproduced, as well as allowing tools
like bugpoint to run properly.

I had to adjust one existing test. The reason for this is that
previously this test would trigger the SDAG-based stack-protector
despite not using llvm.stackguard. With the introduction of
the latter, this pattern is not actually generated by the
StackProtector pass anymore. For now I simply deleted the checks
for the precise generated instructions. Alternatively, it might
make sense to delete the test entirely, since the pattern it
tests for is no longer used.

Diff Detail

Event Timeline

loladiro created this revision.May 11 2018, 6:40 PM

To help myself recovering the memory better, I attempt to rephrase the problem:

  • The IR pass StackProtector always generate the prologue, but not always generate the epilogue.
  • Whether SDAG should generate the epilogue is indicated by StackProtector::shouldEmitSDCheck.
  • During a re-run on llc, it doesn't matter whether we run StackProtector IR pass as part of the llc pipeline, the result is not expected. If we do run StackProtector, it inserts the prologue again; if we don't run StackProtector, StackProtector::shouldEmitSDCheck doesn't return the expected value, as the data members are not set.

If my memory recovey is successful and I understand the problem correctly, the best solution to me is to separate the mutation and analysis code in StackProtector. Suppose we split it into two passes "StackProtectorAnalysis" and "InsertStackProtectorPrologue", will it solve the problem? During the re-run, we only run StackProtectorAnalysis, but never again insert prologue. StackProtectorAnalysis should be kept as-is, without more special logic to detect whether the prologue is already generated (as this patch does). What do you think?

If my memory recovey is successful and I understand the problem correctly, the best solution to me is to separate the mutation and analysis code in StackProtector. Suppose we split it into two passes "StackProtectorAnalysis" and "InsertStackProtectorPrologue", will it solve the problem? During the re-run, we only run StackProtectorAnalysis, but never again insert prologue. StackProtectorAnalysis should be kept as-is, without more special logic to detect whether the prologue is already generated (as this patch does). What do you think?

I think your description is accurate, with the small addition that if shouldEmitSDCheck is wrong (returns true when it shouldn't), the backend crashes because it expects the middle end to have inserted appropriate global declarations which are not there. I think it makes a lot of sense to separate the analysis and the mutation, but I'm not that's entirely sufficient without also giving the analysis the code to detect whether the code was already generated. Also, at the moment the mutation pass is implicitly added to the pass pipeline. Were you proposing that after the split it no longer is? If so, I think that would be quite breaking to users that expect it to be added implicitly. If not, I'm not sure it fixes the issue. Maybe I'm misunderstanding what you are proposing.

My feeling is that getSDagStackGuard/getSSPStackGuardCheck gets mismatched with shouldEmitSDCheck/SupportsSelectionDAGSP. Would you explain the invariance between them if you already investigated it? It seems shouldEmitSDCheck needs to be fixed too, but the current patch doesn't fix it.

Also, the function getSSPStackGuardCheck is weird, since we already have getSDagStackGuard.