This is an archive of the discontinued LLVM Phabricator instance.

[clang][CodeGen] Avoid emitting ifuncs with undefined resolvers
ClosedPublic

Authored by ibookstein on Feb 21 2022, 11:24 AM.

Details

Summary

The purpose of this change is to fix the following codegen bug:

// main.c
__attribute__((cpu_specific(generic)))
int *foo(void) { static int z; return &z;}
int main() { return *foo() = 5; }

// other.c
__attribute__((cpu_dispatch(generic))) int *foo(void);

// run:
clang main.c other.c -o main; ./main

This will segfault prior to the change, and return the correct
exit code 5 after the change.

The underlying cause is that when a translation unit contains
a cpu_specific function without the corresponding cpu_dispatch
the generated code binds the reference to foo() against a
GlobalIFunc whose resolver is undefined. This is invalid: the
resolver must be defined in the same translation unit as the
ifunc, but historically the LLVM bitcode verifier did not check
that. The generated code then binds against the resolver rather
than the ifunc, so it ends up calling the resolver rather than
the resolvee. In the example above it treats its return value as
an int *, therefore trying to write to program text.

The root issue at the representation level is that GlobalIFunc,
like GlobalAlias, does not support a "declaration" state. The
object which provides the correct semantics in these cases
is a Function declaration, but unlike Functions, changing a
declaration to a definition in the GlobalIFunc case constitutes
a change of the object type, as opposed to simply emitting code
into a Function.

I think this limitation is unlikely to change, so I implemented
the fix by returning a function declaration rather than an ifunc
when encountering cpu_specific, and upgrading it to an ifunc
when emitting cpu_dispatch.
This uses takeName + replaceAllUsesWith in similar vein to
other places where the correct IR object type cannot be known
locally/up-front, like in CodeGenModule::EmitAliasDefinition.

Previous discussion in: https://reviews.llvm.org/D112349

Signed-off-by: Itay Bookstein <ibookstein@gmail.com>

Diff Detail

Event Timeline

ibookstein requested review of this revision.Feb 21 2022, 11:24 AM
ibookstein created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2022, 11:24 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

clang-format + description wording

ibookstein edited the summary of this revision. (Show Details)Feb 21 2022, 12:11 PM

So to clarify: The purpose of this patch is to change the call to an 'ifunc' (named FOO.ifunc) to an llvm::Function with the same name? And this later gets replaced by the cpu-dispatch version in the other translation unit? (or, is a linker failure?).

MaskRay added inline comments.Feb 22 2022, 4:01 PM
clang/lib/CodeGen/CodeGenModule.h
358 ↗(On Diff #410360)

Optional: SmallVector<X, 0> typically compiles to less code.

Yeah, that's what happens with this patch; Reference binds against an llvm::Function declaration, linker resolves it to the actual ifunc in another translation unit and therefore emits IFUNC relocation.

Thinking about it more, this is inelegant. I would have liked the reference against the cpu_specific to bind against a plain "FOO" function declaration and not "FOO.ifunc", and 'upgrade' it later once a cpu_dispatch is encountered.
To my understanding, this is actually the reason https://reviews.llvm.org/D67058 added the plain-name alias.

I'll try to see if I can rework that.

Yeah, that's what happens with this patch; Reference binds against an llvm::Function declaration, linker resolves it to the actual ifunc in another translation unit and therefore emits IFUNC relocation.

Thinking about it more, this is inelegant. I would have liked the reference against the cpu_specific to bind against a plain "FOO" function declaration and not "FOO.ifunc", and 'upgrade' it later once a cpu_dispatch is encountered.
To my understanding, this is actually the reason https://reviews.llvm.org/D67058 added the plain-name alias.

I'll try to see if I can rework that.

I was actually in favor of that... The problem is that the cpu-dispatch in a different TU should be what is found, and there is value to having the linker failure in that case. I wouldn't want something like:

TU1:

[[cpu_specific(generic)]]
void foo(){}
void caller() {
foo();
}

TU2:

void foo() {
some weirdo generic impl
}

to work in this case.

The point of that other review was the inverse: someone who DIDN'T know about the multiversioning could get it. We don't want this (not getting multiversioning even though you expect it!) to work.

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

Changed code to generating a function declaration and 'upgrading'
it to an ifunc instead of generating and ifunc and 'downgrading'
it to a function declaration. I decided against changing it to
bind against the unsuffixed alias instead of the ifunc, though,
because that seems to be more involved for little benefit.

Ah, I saw your comment just now, good thing I didn't continue down that plain-alias-name route then!
The change now satisfies that requirement in a way that binding against the alias name indeed would not: TU1 will have the callsite in caller bind against foo.ifunc, which is not a symbol that TU2 defines.

erichkeane accepted this revision.Feb 25 2022, 7:28 AM
This revision is now accepted and ready to land.Feb 25 2022, 7:28 AM

Ah, I saw your comment just now, good thing I didn't continue down that plain-alias-name route then!
The change now satisfies that requirement in a way that binding against the alias name indeed would not: TU1 will have the callsite in caller bind against foo.ifunc, which is not a symbol that TU2 defines.

Ah! I'm glad you did too! Thanks for the patch. I think this works for me, I definitely like the 'upgrade' better than the 'downgrade'.

Can you please make sure there is an 'upgrade' path test in all of the configurations in the tests before committing?

That is, something like:
dispatch def
specific def
call

in all orders?

Add tests for specific>usage>dispatch and dispatch>usage>specific
orderings.

This revision was landed with ongoing or failed builds.Feb 26 2022, 1:18 AM
This revision was automatically updated to reflect the committed changes.