ibookstein created this revision.
ibookstein added reviewers: erichkeane, rsmith, MaskRay.
ibookstein requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
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
(which is a valid use-case, they can be put into different
translation units), 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 the function).
I think this limitation is unlikely to change, so I implemented
the fix by rewriting the generated IR to use a function
declaration instead of an ifunc if the resolver ends up undefined.
This uses takeName + replaceAllUsesWith in similar vein to
other places where the correct IR object type cannot be known
up front locally, like in CodeGenModule::EmitAliasDefinition.
In this case, we don't know whether the translation unit
will contain the cpu_dispatch when generating code for a reference
bound against a cpu_specific symbol.
It is also possible to generate the reference as a Function
declaration first, and 'upgrade' it to a GlobalIFunc once a
cpu_dispatch is encountered, which is somewhat more 'natural'.
That would involve a larger code change, though, so I wanted to
get feedback on the viability of the approach first.
Signed-off-by: Itay Bookstein <ibookst...@gmail.com>
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D120266
Files:
clang/lib/CodeGen/CodeGenModule.cpp
clang/lib/CodeGen/CodeGenModule.h
clang/test/CodeGen/attr-cpuspecific.c
Index: clang/test/CodeGen/attr-cpuspecific.c
===================================================================
--- clang/test/CodeGen/attr-cpuspecific.c
+++ clang/test/CodeGen/attr-cpuspecific.c
@@ -8,6 +8,7 @@
#endif // _MSC_VER
// Each version should have an IFunc and an alias.
+// LINUX: @SingleVersion = weak_odr alias void (), void ()* @SingleVersion.ifunc
// LINUX: @TwoVersions = weak_odr alias void (), void ()* @TwoVersions.ifunc
// LINUX: @TwoVersionsSameAttr = weak_odr alias void (), void ()* @TwoVersionsSameAttr.ifunc
// LINUX: @ThreeVersionsSameAttr = weak_odr alias void (), void ()* @ThreeVersionsSameAttr.ifunc
@@ -18,8 +19,8 @@
// LINUX: @GenericAndPentium = weak_odr alias i32 (i32, double), i32 (i32, double)* @GenericAndPentium.ifunc
// LINUX: @DispatchFirst = weak_odr alias i32 (), i32 ()* @DispatchFirst.ifunc
-// LINUX: @TwoVersions.ifunc = weak_odr ifunc void (), void ()* ()* @TwoVersions.resolver
// LINUX: @SingleVersion.ifunc = weak_odr ifunc void (), void ()* ()* @SingleVersion.resolver
+// LINUX: @TwoVersions.ifunc = weak_odr ifunc void (), void ()* ()* @TwoVersions.resolver
// LINUX: @TwoVersionsSameAttr.ifunc = weak_odr ifunc void (), void ()* ()* @TwoVersionsSameAttr.resolver
// LINUX: @ThreeVersionsSameAttr.ifunc = weak_odr ifunc void (), void ()* ()* @ThreeVersionsSameAttr.resolver
// LINUX: @NoSpecifics.ifunc = weak_odr ifunc void (), void ()* ()* @NoSpecifics.resolver
@@ -34,6 +35,21 @@
// LINUX: define{{.*}} void @SingleVersion.S() #[[S:[0-9]+]]
// WINDOWS: define dso_local void @SingleVersion.S() #[[S:[0-9]+]]
+ATTR(cpu_dispatch(ivybridge))
+void SingleVersion(void);
+// LINUX: define weak_odr void ()* @SingleVersion.resolver()
+// LINUX: call void @__cpu_indicator_init
+// LINUX: ret void ()* @SingleVersion.S
+// LINUX: call void @llvm.trap
+// LINUX: unreachable
+
+// WINDOWS: define weak_odr dso_local void @SingleVersion() comdat
+// WINDOWS: call void @__cpu_indicator_init()
+// WINDOWS: call void @SingleVersion.S()
+// WINDOWS-NEXT: ret void
+// WINDOWS: call void @llvm.trap
+// WINDOWS: unreachable
+
ATTR(cpu_specific(ivybridge))
void NotCalled(void){}
// LINUX: define{{.*}} void @NotCalled.S() #[[S]]
@@ -80,6 +96,10 @@
// CHECK: define {{.*}}void @ThreeVersionsSameAttr.S() #[[S]]
// CHECK: define {{.*}}void @ThreeVersionsSameAttr.Z() #[[K]]
+ATTR(cpu_specific(knl))
+int CpuSpecificNoDispatch(void) {return 1;}
+// CHECK: define {{.*}}i32 @CpuSpecificNoDispatch.Z() #[[K:[0-9]+]]
+
void usages(void) {
SingleVersion();
// LINUX: @SingleVersion.ifunc()
@@ -93,6 +113,9 @@
ThreeVersionsSameAttr();
// LINUX: @ThreeVersionsSameAttr.ifunc()
// WINDOWS: @ThreeVersionsSameAttr()
+ CpuSpecificNoDispatch();
+ // LINUX: @CpuSpecificNoDispatch.ifunc()
+ // WINDOWS: @CpuSpecificNoDispatch()
}
// has an extra config to emit!
Index: clang/lib/CodeGen/CodeGenModule.h
===================================================================
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -42,6 +42,7 @@
class Constant;
class ConstantInt;
class Function;
+class GlobalIFunc;
class GlobalValue;
class DataLayout;
class FunctionType;
@@ -352,6 +353,10 @@
/// we properly emit the iFunc.
std::vector<GlobalDecl> MultiVersionFuncs;
+ /// List of multiversion IFuncs we have emitted. Used to downgrade into
+ /// function declarations when we do not emit a definition for the resolver.
+ std::vector<llvm::GlobalIFunc *> MultiVersionIFuncs;
+
typedef llvm::StringMap<llvm::TrackingVH<llvm::Constant> > ReplacementsTy;
ReplacementsTy Replacements;
@@ -1555,6 +1560,10 @@
void emitMultiVersionFunctions();
+ /// Replace multiversion IFuncs whose resolver is undefined with function
+ /// declarations.
+ void replaceUndefinedMultiVersionIFuncs();
+
/// Emit any vtables which we deferred and still have a use for.
void EmitDeferredVTables();
Index: clang/lib/CodeGen/CodeGenModule.cpp
===================================================================
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -508,6 +508,7 @@
applyReplacements();
checkAliases();
emitMultiVersionFunctions();
+ replaceUndefinedMultiVersionIFuncs();
EmitCXXGlobalInitFunc();
EmitCXXGlobalCleanUpFunc();
registerGlobalDtorsWithAtExit();
@@ -3289,6 +3290,22 @@
EmitGlobalFunctionDefinition(GD, GV);
}
+void CodeGenModule::replaceUndefinedMultiVersionIFuncs() {
+ for (llvm::GlobalIFunc *GIF : MultiVersionIFuncs) {
+ llvm::Function *Resolver = GIF->getResolverFunction();
+ if (!Resolver->isDeclaration())
+ continue;
+
+ auto *DeclTy = cast<llvm::FunctionType>(GIF->getValueType());
+ auto *Decl = llvm::Function::Create(DeclTy, llvm::Function::ExternalLinkage,
+ GIF->getAddressSpace(), "",
+ &getModule());
+ Decl->takeName(GIF);
+ GIF->replaceAllUsesWith(Decl);
+ GIF->eraseFromParent();
+ }
+}
+
void CodeGenModule::EmitGlobalDefinition(GlobalDecl GD, llvm::GlobalValue *GV) {
const auto *D = cast<ValueDecl>(GD.getDecl());
@@ -3663,6 +3680,7 @@
"", Resolver, &getModule());
GIF->setName(ResolverName);
SetCommonAttributes(FD, GIF);
+ MultiVersionIFuncs.push_back(GIF);
return GIF;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
