[IR] Modelling of GlobalIFunc

ns-ibookstein · September 7, 2020, 5:06pm

I was working on ⚙ D81911 [ThinLTO] Work around getBaseObject returning null for alias-to-ifunc to try and fix
46340 – Null dereference when generating module summary index with alias-to-ifunc . Through the review
process we happened upon a design limitation or perhaps a potential
mis-modelling of GlobalIFunc in the IR object hierarchy, which leads
to some problems in LTO flows.

To summarize, as it currently stands (and in the hopes of faithfully
representing the conclusions of that discussion):
* Calling getBaseObject() on a GlobalAlias whose aliasee is a
GlobalIFunc currently returns null.
* Calling getBaseObject() on a GlobalIFunc returns its resolver function.
* This causes computeAliasSummary in ModuleSummaryAnalysis to crash on
a null dereference for an alias-to-ifunc situation.
* A GlobalIFunc and its resolver are *not* interchangeable: at the
interface level, they have different signatures (conceptually, the
IFunc has the same signature of the function pointer that the resolver
potentially returns, not of the resolver itself).
* It makes sense for the IFunc to be its own base object (which is
GlobalObject-like-behavior), but type-hierarchy-wise it can't. This is
because GlobalIFunc derives from GlobalIndirectSymbol which derives
directly from GlobalValue, and therefore it is not a GlobalObject.

Would it make sense for GlobalIFunc to derive from GlobalObject
instead of GlobalIndirectSymbol? If so, GlobalIndirectSymbol would
lose its purpose somewhat, and could be merged into GlobalAlias. It
would, however, require updating a considerable amount of code.

~Itay

teresajohnson · September 8, 2020, 5:22pm

Thanks Itay for summarizing the discussion on D81911. Directly adding a few folks either involved with the discussion there (Dmitry, who originally ported the gcc implementation to clang/llvm), or who have implemented other patches relating to IFunc support in llvm (Peter).

Teresa

dmitry · September 10, 2020, 12:15pm

Calling getBaseObject() on a GlobalIFunc returns its resolver function.

I still don’t understand how it can happen looking to the code (https://github.com/llvm/llvm-project/blob/master/llvm/lib/IR/Globals.cpp#L430). I believe it should return nullptr (the same as if you call it from GlobalAlias that refers to GlobalIFunc).

I don’t have a strong opinion here because deriving GlobalIFunc from GlobalObject does make some sense. At the same time from implementation point of view there were lots of similarities between GlobalAlias and GlobalIFunc (in the most case they should be handled identically or very similarly). getBaseObject method in initial implementation belonged to GlobalAlias only. David moved it to GlobalIndirectSymbol in https://reviews.llvm.org/rG95549497ec8b5269f0439f12859537b7371b7c90 but unfortunately I cannot find corresponding review and discussion.

ns-ibookstein · September 10, 2020, 8:48pm

I still don't understand how it can happen looking to the code (https://github.com/llvm/llvm-project/blob/master/llvm/lib/IR/Globals.cpp#L430). I believe it should return nullptr (the same as if you call it from GlobalAlias that refers to GlobalIFunc).

That is because that's findBaseObject(), not getBaseObject(): if you
call getBaseObject() on a GlobalIFunc you'll get the
GlobalIndirectSymbol::getBaseObject() implementation
(https://github.com/llvm/llvm-project/blob/cb19e8c6d192a108b72ab07362921864a9e244f9/llvm/lib/IR/Globals.cpp#L463),
which passes getOperand(0) - the resolver - as the first parameter to
findBaseObject(). Because the resolver is a Function, findBaseObject()
simply returns it as-is. Just checked in gdb

dmitry · September 11, 2020, 8:56pm

Oh, you are right. My mistake, I expected that findBaseObject is called on this so first hope and other work the same. It would be a good thing to do anyway because it will help to detect loops earlier and make getBaseObject consistent.

dexonsmith · September 8, 2021, 5:03pm

For the specific problem hit below, it feels like another available approach would be to change GlobalIndirectSymbol to behave correctly for both GlobalAlias and GlobalIFunc, without changing the class hierarchy, by reducing its scope and deferring more to its derived classes (e.g., change getBaseObject() to do the right thing).

Can you comment further on the tradeoffs vs. the refactoring you’re proposing? (I see your argument that globalifunc shares some properties globalobject, but it’s not obvious to me that it’s more similar to globalobject than it is to globalalias, or that in aggregate code dealing with these classes will be cleaner after moving globalifunc over to the globalobject bucket.)

ns-ibookstein · September 11, 2021, 10:21am

That really depends on what the correct behavior is for
getBaseObject(). As far as I can tell, this method was originally
added to pierce through GlobalAliases (including various
ConstantExpr-s) and get to the "subject" *GlobalObject* (which is its
return type).

When looking at getBaseObject() through these
GlobalAlias-colored-glasses, I think that the following invariant
should follow:
* getBaseObject() should be idempotent (at least for the cases that it
does not return a nullptr): GV->getBaseObject()->getBaseObject() ==
GV->getBaseObject().
Note that a GlobalAlias may have a different type than its base object
[4], so my previous arguments about a same-type-invariant are
incorrect.

Now, let us consider the case of a GlobalAlias where the aliasee is a
GlobalIFunc. This happens in glibc for example.
What should GA->getBaseObject() return in this case? I think there are
three options here:
1. nullptr; this sort-of-misses the original point of getBaseObject(),
since there *is* an aliasee, you don't even need to pierce through
ConstantExpr-s to get it.
2. The GlobalIFunc itself; this is only possible with either:
2. a. Making GlobalIFunc inherit from GlobalObject [1].
2. b. Changing the return type of getBaseObject() to be GlobalValue
(which also seems to be missing the original point of
getBaseObject()).
3. The GlobalIFunc's resolver; this is similar to the behavior of
getRootObject() in my PR [1].

The current situation on top of master is that GA->getBaseObject()
would return nullptr, but GI->getBaseObject() would return the
resolver, which makes it non-idempotent.
To resolve that, I think that either 2.a. or 3. make sense here.

Some users [2] consider the result of GlobalAlias::getBaseObject() to
be the aliasee [2], which option 3 doesn't uphold.
Other users [3] consider the result of getBaseObject() to be some
"root" / "key" / "leader" for a group of GlobalValues that need to be
handled together, which option 2 doesn't uphold. This is the reason
for the getBaseObject/getRootObject split in my PR, which demonstrates
2.a.

There's also the replication of some of this type hierarchy and
semantics in {GlobalValue,Alias,Function,GlobalVar}Summary::getBaseObject(),
which I haven't yet considered. Note:
* IFunc is missing from that list
* GlobalValueSummary::getBaseObject() returns a GlobalValueSummary,
not a GlobalObjectSummary (which doesn't exist).

Thoughts?

MaskRay · September 14, 2021, 5:26pm

That really depends on what the correct behavior is for
getBaseObject(). As far as I can tell, this method was originally
added to pierce through GlobalAliases (including various
ConstantExpr-s) and get to the "subject" *GlobalObject* (which is its
return type).

When looking at getBaseObject() through these
GlobalAlias-colored-glasses, I think that the following invariant
should follow:
* getBaseObject() should be idempotent (at least for the cases that it
does not return a nullptr): GV->getBaseObject()->getBaseObject() ==
GV->getBaseObject().
Note that a GlobalAlias may have a different type than its base object
[4], so my previous arguments about a same-type-invariant are
incorrect.

Now, let us consider the case of a GlobalAlias where the aliasee is a
GlobalIFunc. This happens in glibc for example.
What should GA->getBaseObject() return in this case? I think there are
three options here:
1. nullptr; this sort-of-misses the original point of getBaseObject(),
since there *is* an aliasee, you don't even need to pierce through
ConstantExpr-s to get it.
2. The GlobalIFunc itself; this is only possible with either:
2. a. Making GlobalIFunc inherit from GlobalObject [1].
2. b. Changing the return type of getBaseObject() to be GlobalValue
(which also seems to be missing the original point of
getBaseObject()).
3. The GlobalIFunc's resolver; this is similar to the behavior of
getRootObject() in my PR [1].

The current situation on top of master is that GA->getBaseObject()
would return nullptr, but GI->getBaseObject() would return the
resolver, which makes it non-idempotent.
To resolve that, I think that either 2.a. or 3. make sense here.

I'd just avoid llvm::GlobalIndirectSymbol::getBaseObject for GlobalIFunc.
getBaseObject can be moved to GlobalAlias:: to avoid GlobalIndirectSymbol confusion.
It seems that most users only call getBaseObject with a GlobalAlias, so the required changes are
few.

For GlobalIFunc, just use getResolver().

Some users [2] consider the result of GlobalAlias::getBaseObject() to
be the aliasee [2], which option 3 doesn't uphold.
Other users [3] consider the result of getBaseObject() to be some
"root" / "key" / "leader" for a group of GlobalValues that need to be
handled together, which option 2 doesn't uphold. This is the reason
for the getBaseObject/getRootObject split in my PR, which demonstrates
2.a.

There's also the replication of some of this type hierarchy and
semantics in {GlobalValue,Alias,Function,GlobalVar}Summary::getBaseObject(),
which I haven't yet considered. Note:
* IFunc is missing from that list
* GlobalValueSummary::getBaseObject() returns a GlobalValueSummary,
not a GlobalObjectSummary (which doesn't exist).

With the changes above, there is probably not much in GlobalIndirectSymbol which can be shared with
GlobalAlias and GlobalIFunc, but that seems ok to me.

dexonsmith · September 14, 2021, 6:13pm

That really depends on what the correct behavior is for
getBaseObject(). As far as I can tell, this method was originally
added to pierce through GlobalAliases (including various
ConstantExpr-s) and get to the “subject” GlobalObject (which is its
return type).

When looking at getBaseObject() through these
GlobalAlias-colored-glasses, I think that the following invariant
should follow:

getBaseObject() should be idempotent (at least for the cases that it
does not return a nullptr): GV->getBaseObject()->getBaseObject() ==
GV->getBaseObject().
Note that a GlobalAlias may have a different type than its base object
[4], so my previous arguments about a same-type-invariant are
incorrect.

Now, let us consider the case of a GlobalAlias where the aliasee is a
GlobalIFunc. This happens in glibc for example.
What should GA->getBaseObject() return in this case? I think there are
three options here:

nullptr; this sort-of-misses the original point of getBaseObject(),
since there is an aliasee, you don’t even need to pierce through
ConstantExpr-s to get it.

The GlobalIFunc itself; this is only possible with either:

a. Making GlobalIFunc inherit from GlobalObject [1].

b. Changing the return type of getBaseObject() to be GlobalValue
(which also seems to be missing the original point of
getBaseObject()).

The GlobalIFunc’s resolver; this is similar to the behavior of
getRootObject() in my PR [1].

The current situation on top of master is that GA->getBaseObject()
would return nullptr, but GI->getBaseObject() would return the
resolver, which makes it non-idempotent.
To resolve that, I think that either 2.a. or 3. make sense here.

I’d just avoid llvm::GlobalIndirectSymbol::getBaseObject for GlobalIFunc.
getBaseObject can be moved to GlobalAlias:: to avoid GlobalIndirectSymbol confusion.
It seems that most users only call getBaseObject with a GlobalAlias, so the required changes are
few.

For GlobalIFunc, just use getResolver().

MaskRay, are you suggesting removing GlobalValue::getBaseObject, forcing an initial cast to GlobalAlias? I agree that sounds like a simple approach.

If there’s a need to keep GlobalValue::getBaseObject, another option would be to return a utility that stands in for ifunc-or-object (wrapping GlobalValue* but requiring it not be a GlobalAlias*). But sinking it to GlobalAlias is simpler if it’ll work.

Itay/others, WDYT?

MaskRay · September 14, 2021, 11:05pm

That really depends on what the correct behavior is for
getBaseObject(). As far as I can tell, this method was originally
added to pierce through GlobalAliases (including various
ConstantExpr-s) and get to the "subject" *GlobalObject* (which is its
return type).

When looking at getBaseObject() through these
GlobalAlias-colored-glasses, I think that the following invariant
should follow:
* getBaseObject() should be idempotent (at least for the cases that it
does not return a nullptr): GV->getBaseObject()->getBaseObject() ==
GV->getBaseObject().
Note that a GlobalAlias may have a different type than its base object
[4], so my previous arguments about a same-type-invariant are
incorrect.

Now, let us consider the case of a GlobalAlias where the aliasee is a
GlobalIFunc. This happens in glibc for example.
What should GA->getBaseObject() return in this case? I think there are
three options here:
1. nullptr; this sort-of-misses the original point of getBaseObject(),
since there *is* an aliasee, you don't even need to pierce through
ConstantExpr-s to get it.
2. The GlobalIFunc itself; this is only possible with either:
2. a. Making GlobalIFunc inherit from GlobalObject [1].
2. b. Changing the return type of getBaseObject() to be GlobalValue
(which also seems to be missing the original point of
getBaseObject()).
3. The GlobalIFunc's resolver; this is similar to the behavior of
getRootObject() in my PR [1].

The current situation on top of master is that GA->getBaseObject()
would return nullptr, but GI->getBaseObject() would return the
resolver, which makes it non-idempotent.
To resolve that, I think that either 2.a. or 3. make sense here.

I'd just avoid llvm::GlobalIndirectSymbol::getBaseObject for GlobalIFunc.
getBaseObject can be moved to GlobalAlias:: to avoid GlobalIndirectSymbol confusion.
It seems that most users only call getBaseObject with a GlobalAlias, so the required changes are
few.

For GlobalIFunc, just use getResolver().

MaskRay, are you suggesting removing GlobalValue::getBaseObject, forcing an initial cast to GlobalAlias? I agree that sounds like a simple approach.

If there’s a need to keep GlobalValue::getBaseObject, another option would be to return a utility that stands in for ifunc-or-object (wrapping `GlobalValue*` but requiring it not be a `GlobalAlias*`). But sinking it to GlobalAlias is simpler if it’ll work.

Itay/others, WDYT?

I created ⚙ D109792 Resolve {GlobalValue,GloalIndirectSymol}::getBaseObject confusion

GlobalValue::getBaseObject has 7 references. I just removed the
confusing GlobalIFunc part from it, but does not make further
refactoring on it.

ns-ibookstein · September 15, 2021, 12:04pm

I've commented the same on the D109792, but I'll also include it here
for completeness:

As I understand it, the problem with that approach is that some
callers rely on the ConstantExpr peeling logic that findBaseObject()
grants to users of getBaseObject(), even for GlobalIFunc-s (where the
resolver may be "hidden" behind bitcasts for example, see e.g.
Compiler Explorer). It is unclear to me whether
all the ConstantExpr types that findBaseObject() handles are required
by "GlobalIFunc::getBaseObject()" API clients, but even in the
simplest of cases clang emits the resolver behind a bitcast at the
very least.

In addition, I think it doesn't address the modelling discrepancy
where one currently cannot retrieve an aliasee if that aliasee happens
to be a GlobalIFunc (cf. option 2 in my previous mail). One can call
getAliasee(), obviously, but then one forgoes the findBaseObject()
peeling recursion and only has a pointer to a Constant at hand.

~Itay