Page MenuHomePhabricator

[IR] remove assert since always_inline can appear on CallBase
ClosedPublic

Authored by nickdesaulniers on Jun 25 2021, 12:38 PM.

Details

Summary

I added an assertion in D91816 (documenting behavior added in D93422)
that callers and callees with mismatched fn attr's related to stack
protectors should not occur unless the callee was attributed
always_inline.

This falls apart when a call, invoke, or callbr (any instruction
inheriting from CallBase) itself has an always_inline attribute. Clang
will emit such attributes on Instructions when attribute((flatten))
is used to recursively force inlining from a caller.

Since these assertions only had the caller and callee Functions, and not
the call site (CallBase derived classes), we would have to search the
caller for such instructions to reconstruct the call site information.
But at that point, inlining has already occurred; the call site has
already been removed from the caller.

Remove the assertions, add a unit test for always_inline call sites, and
update the LangRef.

Another curiosity is that the always_inline Attribute on Instructions is
only expanded by the inline pass, not the always_inline pass.

Thanks to @pcc on this report when building Android's RunTime (ART)
interpreter.

Diff Detail

Event Timeline

nickdesaulniers requested review of this revision.Jun 25 2021, 12:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 25 2021, 12:38 PM
nickdesaulniers edited the summary of this revision. (Show Details)Jun 25 2021, 12:39 PM
nickdesaulniers planned changes to this revision.Jun 25 2021, 3:06 PM

I've added optimization remark tests locally; inlining is failing due to:

remark: <unknown>:0:0: ssp not inlined into nossp because it should never be inlined (cost=never): stack protected callee but caller requested no stack protector
remark: <unknown>:0:0: ssp not inlined into nossp_alwaysinline because it should never be inlined (cost=never): stack protected callee but caller requested no stack protector
remark: <unknown>:0:0: ssp has uninlinable pattern (dynamic alloca) and cost is not fully computed
remark: <unknown>:0:0: ssp not inlined into nossp_caller because it should never be inlined (cost=never): dynamic alloca
remark: <unknown>:0:0: nossp has uninlinable pattern (dynamic alloca) and cost is not fully computed
remark: <unknown>:0:0: nossp not inlined into ssp2 because it should never be inlined (cost=never): dynamic alloca

the dynamic allocas mean this test, as introduced in D91816, might not be testing the right thing. We were getting the right results, but for the wrong reasons. Let me attempt to fix that first, then revisit this. It's confusing to me at the moment because the dynamic allocas are the whole point of having stack protectors, and we can and do inline such code, so it's not initially clear to my why inlining would fail due to that, at least with the tests as written.

Let me rebase this on D104958.

pcc accepted this revision.Jun 28 2021, 12:43 PM

LGTM

This revision is now accepted and ready to land.Jun 28 2021, 12:43 PM

FWIW, this seems sensible :)

MaskRay accepted this revision.Jun 28 2021, 12:55 PM
This revision was landed with ongoing or failed builds.Jun 28 2021, 1:59 PM
This revision was automatically updated to reflect the committed changes.