[UniqueLinkageName] Use exsiting GlobalDecl object instead of reconstructing one.

[hoy]

Summary

C++ constructors/destructors need to go through a different constructor to construct a GlobalDecl object in order to retrieve their linkage type. This causes an assert failure in the default constructor of GlobalDecl. I'm chaning it to using the exsiting GlobalDecl object.

[dblaikie]

This was previously crashing, I guess? Testing should validate the behavior beyond the crash, though - (presumably there's some more specific behavior than "does not crash" that wasn't being tested for before - that certain names are mangled appropriately or what-have-you)

[hoy]

In D102356#2758179, @dblaikie wrote:

This was previously crashing, I guess? Testing should validate the behavior beyond the crash, though - (presumably there's some more specific behavior than "does not crash" that wasn't being tested for before - that certain names are mangled appropriately or what-have-you)

Yes, an assert was triggered related to c++ constructors/destructors while it's not now. Regarding the behavior, c++ constructors/destructors are not static, so I don't expect a behavior change.

[dblaikie]

In D102356#2758371, @hoy wrote:

In D102356#2758179, @dblaikie wrote:

This was previously crashing, I guess? Testing should validate the behavior beyond the crash, though - (presumably there's some more specific behavior than "does not crash" that wasn't being tested for before - that certain names are mangled appropriately or what-have-you)

Yes, an assert was triggered related to c++ constructors/destructors while it's not now. Regarding the behavior, c++ constructors/destructors are not static, so I don't expect a behavior change.

ctors/dtors can have internal linkage, if the type has internal linkage (if it's in an anonymous namespace) - but in any case, my point was that there's some specific behavior you're expecting, even if that behavior is "does not get this attribute" - previously no code tested that with this feature enabled the ctor wouldn't get the attribute (because it couldn't've tested that, because what it did was crash) - so testing that would be good.

But testing the attribute does work on the ctor of a type in an anonymous namespace would be suitable too.

[hoy]

In D102356#2760973, @dblaikie wrote:

In D102356#2758371, @hoy wrote:

In D102356#2758179, @dblaikie wrote:

This was previously crashing, I guess? Testing should validate the behavior beyond the crash, though - (presumably there's some more specific behavior than "does not crash" that wasn't being tested for before - that certain names are mangled appropriately or what-have-you)

Yes, an assert was triggered related to c++ constructors/destructors while it's not now. Regarding the behavior, c++ constructors/destructors are not static, so I don't expect a behavior change.

ctors/dtors can have internal linkage, if the type has internal linkage (if it's in an anonymous namespace) - but in any case, my point was that there's some specific behavior you're expecting, even if that behavior is "does not get this attribute" - previously no code tested that with this feature enabled the ctor wouldn't get the attribute (because it couldn't've tested that, because what it did was crash) - so testing that would be good.

But testing the attribute does work on the ctor of a type in an anonymous namespace would be suitable too.

Good to know that ctors/dtors can have internal linkage. How do you get that? The C++ standard doesn't allow ctors to be static.

[dblaikie]

In D102356#2761010, @hoy wrote:

In D102356#2760973, @dblaikie wrote:

In D102356#2758371, @hoy wrote:

In D102356#2758179, @dblaikie wrote:

This was previously crashing, I guess? Testing should validate the behavior beyond the crash, though - (presumably there's some more specific behavior than "does not crash" that wasn't being tested for before - that certain names are mangled appropriately or what-have-you)

Yes, an assert was triggered related to c++ constructors/destructors while it's not now. Regarding the behavior, c++ constructors/destructors are not static, so I don't expect a behavior change.

ctors/dtors can have internal linkage, if the type has internal linkage (if it's in an anonymous namespace) - but in any case, my point was that there's some specific behavior you're expecting, even if that behavior is "does not get this attribute" - previously no code tested that with this feature enabled the ctor wouldn't get the attribute (because it couldn't've tested that, because what it did was crash) - so testing that would be good.

But testing the attribute does work on the ctor of a type in an anonymous namespace would be suitable too.

Good to know that ctors/dtors can have internal linkage. How do you get that? The C++ standard doesn't allow ctors to be static.

Placing the type within an anonymous namespace should do the trick, something like this: https://godbolt.org/z/8YMWhbWM8

[hoy]

Added tests for static ctor/dtor.

[dblaikie]

Looks alright, thanks!

clang/lib/CodeGen/CGCall.cpp

if (CodeGenOpts.NoPLT) {
       }
     }
   }
 
   // Add "sample-profile-suffix-elision-policy" attribute for internal linkage
   // functions with -funique-internal-linkage-names.
   if (TargetDecl && CodeGenOpts.UniqueInternalLinkageNames) {
     if (auto *Fn = dyn_cast<FunctionDecl>(TargetDecl)) {
-      if (this->getFunctionLinkage(Fn) == llvm::GlobalValue::InternalLinkage)
+      if (this->getFunctionLinkage(CalleeInfo.getCalleeDecl()) ==
+          llvm::GlobalValue::InternalLinkage)
         FuncAttrs.addAttribute("sample-profile-suffix-elision-policy",
                                "selected");
     }
   }
 
   // Collect non-call-site function IR attributes from declaration-specific
   // information.
   if (!AttrOnCallSite) {

clang/test/CodeGen/unique-internal-linkage-names.cpp

 __attribute__((target("sse4.2"))) static int mver() {
   return 1;
 }
 
 int mver_call() {
   return mver();
 }
 
+namespace {
+class A {
+public:
+  A() {}
+  ~A() {}
+};
+}
+
+void test() {
+  A a;
+}
+
 // PLAIN: @_ZL4glob = internal global
 // PLAIN: @_ZZ8retAnonMvE5fGlob = internal global
 // PLAIN: @_ZN12_GLOBAL__N_16anon_mE = internal global
 // PLAIN: define internal i32 @_ZL3foov()
 // PLAIN: define internal i32 @_ZN12_GLOBAL__N_14getMEv
 // PLAIN: define weak_odr i32 ()* @_ZL4mverv.resolver()
+// PLAIN: define internal void @_ZN12_GLOBAL__N_11AC1Ev
+// PLAIN: define internal void @_ZN12_GLOBAL__N_11AD1Ev
 // PLAIN: define internal i32 @_ZL4mverv()
 // PLAIN: define internal i32 @_ZL4mverv.sse4.2()
 // PLAIN-NOT: "sample-profile-suffix-elision-policy"
 // UNIQUE: @_ZL4glob = internal global
 // UNIQUE: @_ZZ8retAnonMvE5fGlob = internal global
 // UNIQUE: @_ZN12_GLOBAL__N_16anon_mE = internal global
 // UNIQUE: define internal i32 @_ZL3foov.[[MODHASH:__uniq.[0-9]+]]() #[[#ATTR:]] {
 // UNIQUE: define internal i32 @_ZN12_GLOBAL__N_14getMEv.[[MODHASH]]
 // UNIQUE: define weak_odr i32 ()* @_ZL4mverv.[[MODHASH]].resolver()
+// UNIQUE: define internal void @_ZN12_GLOBAL__N_11AC1Ev.__uniq.68358509610070717889884130747296293671
+// UNIQUE: define internal void @_ZN12_GLOBAL__N_11AD1Ev.__uniq.68358509610070717889884130747296293671
 // UNIQUE: define internal i32 @_ZL4mverv.[[MODHASH]]()
 // UNIQUE: define internal i32 @_ZL4mverv.[[MODHASH]].sse4.2
 // UNIQUE: attributes #[[#ATTR]] = { {{.*}}"sample-profile-suffix-elision-policy"{{.*}} }