This is an archive of the discontinued LLVM Phabricator instance.

[IR] Refactor GlobalIFunc to inherit from GlobalObject, Remove GlobalIndirectSymbol
ClosedPublic

Authored by ibookstein on Aug 28 2021, 9:15 AM.

Details

Summary

As discussed in:

The GlobalIndirectSymbol class lost most of its meaning in
https://reviews.llvm.org/D109792, which disambiguated getBaseObject
(now getAliaseeObject) between GlobalIFunc and everything else.
In addition, as long as GlobalIFunc is not a GlobalObject and
getAliaseeObject returns GlobalObjects, a GlobalAlias whose aliasee
is a GlobalIFunc cannot currently be modeled properly. Creating
aliases for GlobalIFuncs does happen in the wild (e.g. glibc). In addition,
calling getAliaseeObject on a GlobalIFunc will currently return nullptr,
which is undesirable because it should return the object itself for
non-aliases.

This patch refactors the GlobalIFunc class to inherit directly from
GlobalObject, and removes GlobalIndirectSymbol (while inlining the
relevant parts into GlobalAlias and GlobalIFunc). This allows for
calling getAliaseeObject() on a GlobalIFunc to return the GlobalIFunc
itself, making getAliaseeObject() more consistent and enabling
alias-to-ifunc to be properly modeled in the IR.

I exercised some judgement in the API clients of GlobalIndirectSymbol:
some were 'monomorphized' for GlobalAlias and GlobalIFunc, and
some remained shared (with the type adapted to become GlobalValue).

Diff Detail

Event Timeline

ibookstein created this revision.Aug 28 2021, 9:15 AM
ibookstein requested review of this revision.Aug 28 2021, 9:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2021, 9:15 AM
ibookstein updated this revision to Diff 371330.Sep 8 2021, 7:20 AM

Fixed the clang-tidy errors in GlobalIFunc.h.

Just came across this; still thinking about it.

A couple of thoughts, assuming the direction gains consensus:

  • It seems like many of the changes in the patch to use GlobalAlias and/or GlobalIFunc directly instead of GlobalIndirectSymbol could be committed separately / ahead of the change to the IR class hierarchy. (Maybe not worth splitting out until direction has consensus, but would be great to do at that point, to isolate the change as much as possible.)
  • Would there be a benefit in leaving behind a GlobalIndirectSymbol utility, modeling a union between GlobalAlias and GlobalIFunc? (I seem to remember a CallSite utility before CallInst and InvokeInst shared the common base class CallBase... but I'm not sure if it'd be similarly useful here.)
llvm/unittests/IR/ConstantsTest.cpp
388–389

It's not obvious how this change is related to the patch... did it slip in accidentally?

ibookstein added a subscriber: respindola.EditedSep 8 2021, 10:29 AM

Regarding leaving GlobalIndirectSymbol as a utility class, it seems to me that its utility is in direct proportion to the number of places I ended up converting to use GlobalValue rather than monomorphizing (to the degree that you agree with my tactical decisions in these places, of course :) ).
The major ones were the BitcodeReader, LLParser, IRLinker, SplitModule, ValueMapper.

Regarding the direction gaining consensus in general, I'm wondering myself whether the benefits of this direction outweigh the costs, I mainly wanted a working patch so that the discussion will no longer be theoretical.
Who would we want to weigh in / involve? I think the opinion of someone who understands the reasoning and design behind the GlobalValue <-> GlobalObject <-> GlobalAlias split will be valuable.
Git names @respindola as the author of that split.

Regarding the build error, looks like clang-format disapproves of the indentation in the LLVM_FOR_EACH_VALUE_SUBCLASS macro (I tried preserving its correlation with the class hierarchy).

llvm/unittests/IR/ConstantsTest.cpp
388–389

It's not accidental, but it could definitely be committed separately. It's a bugfix for a type mismatch error that was exposed by the change to the IR class hierarchy, since only afterwards does the code-path of GlobalAlias::create pass through the type check assertion in GlobalAlias::setAliasee:

void GlobalAlias::setAliasee(Constant *Aliasee) {
  assert((!Aliasee || Aliasee->getType() == getType()) &&
         "Alias and aliasee types should match!");
  ...
}

Before the change, the GlobalAlias::create code-path sets the aliasee via the GlobalIndirectSymbol::GlobalIndirectSymbol constructor, which unceremoniously sets Op<0>() = Symbol; without performing the type check (which it can't, because for GlobalIFunc that check would be incorrect).

ibookstein edited the summary of this revision. (Show Details)

Rebased past merge of https://reviews.llvm.org/D109792
Removed getRootObject

ibookstein updated this revision to Diff 377838.Oct 7 2021, 7:12 AM
ibookstein added a reviewer: MaskRay.

Rebased past merge of getBaseObject->getAliaseeObject rename.

The current behavior of GlobalValue::getBaseObject is inconsistent. For example, calling getBaseObject on a GlobalAlias whose Aliasee

This description needs update after renaming of getBaseObject.

llvm/docs/LangRef.rst
910 ↗(On Diff #377838)

Drop the change?

An ifunc definition can be interposed/preempted.

ibookstein edited the summary of this revision. (Show Details)Oct 7 2021, 10:37 AM
ibookstein added inline comments.Oct 7 2021, 10:46 AM
llvm/docs/LangRef.rst
910 ↗(On Diff #377838)

The change only documents the existing situation, though, as far as I can tell.
There are tests which check/specify a [PreemptionSpecifier]; If I remember correctly, if you'd drop the PrintDSOLocation/maybeSetDSOLocal you'd get test failures.
I searched for "dso_local ifunc" and got some hits from the tests.

Tiny clang-format fix on getGVPartitioningRoot.

MaskRay accepted this revision.Oct 7 2021, 11:25 AM

Thanks. This makes sense. In the inheritance hierachy, GlobalIndirectSymbol does nothing useful now and can just be removed.

I'll wait a few days and commit on your behalf.

llvm/docs/LangRef.rst
910 ↗(On Diff #377838)

If that's the case. The documentation change can be pushed separately to reflect the fact that it is unrelated to this change.

Pushed f66b1b2717e84edef8a9e132638de6d866d01eab

llvm/lib/AsmParser/LLParser.cpp
1067

Is this unneeded now?

llvm/lib/Transforms/IPO/LowerTypeTests.cpp
373
This revision is now accepted and ready to land.Oct 7 2021, 11:25 AM
ibookstein added inline comments.Oct 7 2021, 11:53 AM
llvm/lib/AsmParser/LLParser.cpp
1067

Note that GV is now a borrowed reference from either GA or GI, which are both std::unique_ptr-s. Their ownership is transferred into the Module in the release() calls in the 5 lines immediately above.

llvm/unittests/IR/ConstantsTest.cpp
388–389

@MaskRay this can also be pushed separately if you'd like

Fixed brace style comment.

This needs a rebase now.

MaskRay updated this revision to Diff 378775.Oct 11 2021, 1:16 PM

clang-format

rebase

MaskRay added a comment.EditedOct 11 2021, 1:23 PM

@ibookstein clang/lib/CodeGen/CodeGenModule.cpp has a use which hasn't been updated. ninja check-clang to run tests.

ibookstein added a comment.EditedOct 11 2021, 2:24 PM

Right, only had llvm enabled so missed that. Looks like it requires a bit of untangling there, will take a deeper look tomorrow.

Ugh. should clang accept situations where the resolver of an ifunc is also an ifunc? The semantics are confusing, but neither compiler diagnoses anything:

itay ~/tmp/gis> cat attr-ifunc.c
int qux() { return 44; }
int (*foo_resolver())() { return qux; }
int foo() __attribute__((ifunc("foo_resolver")));

int (*(*bar_resolver_resolver())())() { return foo_resolver; }
int (*bar_resolver())() __attribute__((ifunc("bar_resolver_resolver")));
int bar() __attribute__((ifunc("bar_resolver")));

int main() { return foo() + bar(); }
itay ~/tmp/gis> gcc attr-ifunc.c -o attr-ifunc -g ; ./attr-ifunc
itay ~/tmp/gis [SIGTTIN]> clang-14 attr-ifunc.c -o attr-ifunc -g ; ./attr-ifunc
itay ~/tmp/gis [108]>

I'm not completely satisfied with the clang fix, but it passes the existing tests. It preserves the existing behavior of getAliasedGlobal, which drills through ifuncs even in alias-to-ifunc and ifunc-to-ifunc (?!) situations.
I think that following the addition of getResolverFunction, it makes sense for clang and llvm bitcode verifier to diagnose and reject situations where the resolver is not a function or an alias to one, and amend the tests in clang/test/Sema/attr-ifunc.c accordingly.

This revision was landed with ongoing or failed builds.Oct 20 2021, 10:29 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2021, 10:29 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
llvm/include/llvm/IR/GlobalIFunc.h