Page MenuHomePhabricator

[GlobalISel] Fix insertion of stack-protector epilogue
ClosedPublic

Authored by petpav01 on Nov 14 2018, 2:07 AM.

Details

Summary

SelectionDAG implements an optimization for stack-protector which causes the StackProtector pass to only insert prologue instrumentation but generating epilogue instrumentation is deferred to SelectionDAG.

The StackProtector pass does the same for GlobalISel but this selection framework currently does not implement the same optimization as SelectionDAG which means no epilogue instrumentation gets inserted by the compiler. This primarily affects AArch64 at -O0 because this configuration enables GlobalISel by default.

The proposed patch fixes the immediate problem by having the StackProtector pass generate the epilogue instrumentation when GlobalISel is enabled, similarly to what is done when FastISel is active.

Notes:

  • The patch changes meaning of TargetOptions::EnableGlobalISel. The flag was previously set only when a target switched on GlobalISel but it is now always set when the GlobalISel pipeline is enabled. This makes the flag consistent with TargetOptions::EnableFastISel and allows its use in StackProtector to determine when GlobalISel is enabled.

    The EnableGlobalISel flag had previouly only one use in TargetPassConfig::isGlobalISelAbortEnabled(). The method used its value to determine whether GlobalISel was enabled by a target and returned false in such a case. To preserve the current behaviour, a new flag TargetOptions::GlobalISelAbort is introduced to separately record the abort behaviour.
  • StackProtector::InsertStackProtectors() is updated to find a stack guard slot by searching for the llvm.stackprotector intrinsic when the prologue was not created by StackProtector itself but the pass still needs to generate the epilogue instrumentation. This fixes a problem when the pass would abort because the stack guard AllocInst pointer was null when generating the epilogue. This was hit by test/CodeGen/AArch64/GlobalISel/arm64-irtranslator-stackprotect.ll.

Diff Detail

Repository
rL LLVM

Event Timeline

petpav01 created this revision.Nov 14 2018, 2:07 AM
aemerson accepted this revision.Nov 28 2018, 3:03 PM

Hi Petr,

This looks ok to me, can you separate this into two separate commits though? One for the option changes and the other for the SP fix.

Thanks,
Amara

test/CodeGen/AArch64/GlobalISel/irtranslator-stackprotect-check.ll
1 ↗(On Diff #174002)

We don't yet officially support GISel on AArch64 except for -O0, so can you add this to the run line?

This revision is now accepted and ready to land.Nov 28 2018, 3:03 PM
This revision was automatically updated to reflect the committed changes.

Thanks Amara. Committed with the suggested change and splitting up the patch as rL347861 and rL347862.

Hello,

r347862 of this change broke the ability to run the test suite with the ARM64 backend enabled BUT without the ARM backend enabled. Was this intentional? See below:

Dave


FAIL: LLVM :: CodeGen/AArch64/GlobalISel/irtranslator-stackprotect-check.ll (18353 of 44268)

  • TEST 'LLVM :: CodeGen/AArch64/GlobalISel/irtranslator-stackprotect-check.ll' FAILED ****

Script:

: 'RUN: at line 1'; /home/dave/s/lc/t/bin/llc -O0 -stop-before=irtranslator -global-isel /home/dave/s/lc/test/CodeGen/AArch64/GlobalISel/irtranslator-stackprotect-check.ll -o - | /home/dave/s/lc/t/bin/FileCheck /home/dave/s/lc/test/CodeGen/AArch64/GlobalISel/irtranslator-stackprotect-check.ll

: 'RUN: at line 2'; /home/dave/s/lc/t/bin/llc -O0 -stop-after=irtranslator -verify-machineinstrs -global-isel /home/dave/s/lc/test/CodeGen/AArch64/GlobalISel/irtranslator-stackprotect-check.ll -o - | /home/dave/s/lc/t/bin/FileCheck --check-prefixes CHECK,CHECK-MIR /home/dave/s/lc/test/CodeGen/AArch64/GlobalISel/irtranslator-stackprotect-check.ll

Exit Code: 2

Command Output (stderr):

/home/dave/s/lc/t/bin/llc: error: : error: unable to get target for 'armv8-arm-none-eabi', see --version and --triple.
FileCheck error: '-' is empty.
FileCheck command line: /home/dave/s/lc/t/bin/FileCheck /home/dave/s/lc/test/CodeGen/AArch64/GlobalISel/irtranslator-stackprotect-check.ll

Apologies for the breakage. This was supposed to be an AArch64 test but the triple is incorrect. Should be fixed in rL348111.