This is an archive of the discontinued LLVM Phabricator instance.

[AIX] Enable stackprotect feature
ClosedPublic

Authored by jsji on May 25 2021, 9:48 AM.

Details

Summary

AIX use __ssp_canary_word instead of __stack_chk_guard.
This patch update the target hook to use correct symbol,
so that the basic stackprotect feature can work.

The traceback will be handled in follow up patch.

Diff Detail

Event Timeline

jsji created this revision.May 25 2021, 9:48 AM
jsji requested review of this revision.May 25 2021, 9:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2021, 9:48 AM
hubert.reinterpretcast added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
16411–16412

Should this be a macro? A file-scope static const char[] will do.

16423

There's a number of instances of hard-coded "__stack_chk_guard" in what appears to be common code, e.g., in llvm/lib/Transforms/IPO/Internalize.cpp. I think those fall within the scope of this patch.

jsji updated this revision to Diff 347760.May 25 2021, 12:36 PM

Address comments.

jsji marked an inline comment as done.May 25 2021, 12:39 PM
jsji added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
16423

I think InternalizePass is not used by default pass builder.
I can update it in a later patch if we really want to support it as well.

hubert.reinterpretcast edited the summary of this revision. (Show Details)May 25 2021, 2:01 PM
jsji added inline comments.May 25 2021, 2:23 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
16423

https://reviews.llvm.org/D103043 just landed so that it is easier to check triple now, I will create a follow up patch later (maybe after a few days when https://reviews.llvm.org/D103043 stabilized). @hubert.reinterpretcast

hubert.reinterpretcast added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
16423

InternalizePass is used by LTO afaik; @w2yehia, can you comment?

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
133

Not declaring static explicitly means that some header somewhere declaring this as extern will cause this to be an external definition. I don't think that's great (and I don't think the case is strong for this file to contain a potentially-external definition of this name).

16409

Why remove the old comment? The code still does override and only use the base implementation for non-Linux.

jsji updated this revision to Diff 347830.May 25 2021, 5:27 PM

Address comments.

w2yehia added inline comments.May 25 2021, 7:02 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
16423

Yes, libLTO uses it directly, outside of any pass manager:

lto_codegen_compile_to_file
  LTOCodeGenerator::compile_to_file
    LTOCodeGenerator::optimize
      LTOCodeGenerator::applyScopeRestrictions
        llvm::internalizeModule
          InternalizePass(std::move(MustPreserveGV)).internalizeModule(TheModule, CG);
jsji updated this revision to Diff 347836.May 25 2021, 8:06 PM

Handle Internalize.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
16423

Thanks Wael.

shchenz added inline comments.May 26 2021, 10:11 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
16424

Is it better to add an assertion here to indicate that the return value will never be nullptr?

llvm/test/CodeGen/PowerPC/stack-guard-oob.ll
2

Should we add the RUN line for 32-bit AIX? I think the enablement is also helpful for 32-bit.

Another nice to have is: should we add RUN line for the other file which is for stack protector on PowerPC on Linux? llvm/test/CodeGen/PowerPC/stack-protector.ll.

If we do this, we can know better about the difference between stack protectors on AIX and Linux?

jsji updated this revision to Diff 348256.May 27 2021, 7:16 AM

Address comments.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
16424

We can, but I don't think it is necessary.
We call M.getOrInsertGlobal in insertSSPDeclarations above, so there should not be nullptr.
Also none of the other targets like x86/arm adding the assert too.

shchenz accepted this revision.May 27 2021, 5:44 PM

LGTM. Thanks.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
16424

OK. I suggest this because some users of this function directly dereference the result without checking null.

This revision is now accepted and ready to land.May 27 2021, 5:44 PM
This revision was landed with ongoing or failed builds.May 27 2021, 7:47 PM
This revision was automatically updated to reflect the committed changes.