Refactor implementation of -funique-internal-linkage-names.

[tmsriram]

Summary

The option -funique-internal-linkage-names was added in D73307 and D78243 as a LLVM early pass to insert a unique suffix to internal linkage functions and vars. The unique suffix was the hash of the module path. However, we found that this can be done more cleanly in clang early and the fixes that need to be done later can be completely avoided. The fixes in particular are trying to modify the DW_AT_linkage_name and finding the right place to insert the pass.

This patch ressurects the original implementation proposed in D73307 which was reviewed and then ditched in favor of the pass based approach.

[hoy]

Thanks for moving this feature into the front end. It looks quite clean.

clang/lib/CodeGen/CGCall.cpp

2093

Can a test be made for this?

clang/lib/CodeGen/CodeGenModule.cpp

205

Will this have a unique dwarf debug name automatically generated? I'm wondering if we should make a test for this.

[dblaikie]

(might be simpler to add the new functionality to Clang, having Clang opt out of the old functionality (to avoid both unique prefixes being added together, I guess?) and then separately remove the LLVM functionality in a follow-up patch, once it's unused by Clang.

clang/lib/CodeGen/CodeGenModule.cpp

202

Can remove some syntax here:

[wmi]

It is a good move. I see that in old pass manager, the initialization phases of all passes run before the body of any pass starts, so for many passes, they will see different names for the same symbol in initialization phase and main phase. SampleProfileLoaderPass is affected for example. It is less of a problem for new pass manager. However, the requirement of putting UniqueInternalLinkageNamesPass very early in the pipeline is still a very strong requirement. It is possible for other pass to have the same requirement and to cause conflict. So like to see the refactoring and thanks for the improvement!

[rnk]

clang/lib/CodeGen/CodeGenModule.cpp

1266

This raises the original concern that I had that pushed me in the direction of doing things as a pass: How does this interact with multi-version functions? It probably just works, but I remember this was something that made me think an IR pass would be simpler, or more likely to do the right thing.

Something else to think about: clang has other ways to generate internal functions with non-unique names. Dynamic initialization comes to mind (__cxx_global_var_init). Consider:

template <typename T>
struct Foo { static int gv; };
int f(int);
template <typename T>
int Foo<T>::gv = f(sizeof(T));
void escape(int*);
void instantiate() {
  escape(&Foo<int>::gv);
  escape(&Foo<short>::gv);
}

The IR pass was renaming these for us previously.

[tmsriram]

clang/lib/CodeGen/CodeGenModule.cpp

1266

I had a test for multi versioned functions, clang/test/CodeGen/unique-internal-linkage-names.cpp. Here is the gist though. foo() with multi-version clones for sse4.2 and avx will produce the following function symbols:

1. _Z3foov.__uniq.1234
2. _Z3foov.sse4.2.__uniq.1234
3. _Z3foov.avx.__uniq.1234
4. _Z3foov.__uniq.1234.resolver

Multi-versioned suffixes are not demangler friendly in general as such, even without this patch. For instance, _Z3foov.sse4.2 will not demangle, Is there a specific concern?

[rnk]

clang/lib/CodeGen/CodeGenModule.cpp

1266

Mainly, I'd just like to keep that test coverage. The logic moved to the frontend, so now that should become a frontend test.

Just from reading the patch, I'm guessing the names now become _Z3foov.__uniq.1234.avx/sse/etc, but I could be wrong. It would be good to encode our expectations one way or another.

[tmsriram]

clang/lib/CodeGen/CodeGenModule.cpp

1266

I like the module hash coming before the multi-version target suffix too like you pointed out. I just need to re-order the code in getMangledNameImpl. I will do that.

[tmsriram]

This patch update handles the following:

1. For C internal linkage functions which do not mangle implicitly, force mangling when -funique-internal-linkage-names is present before adding the suffix. This ensures demanglers work correctly on the linkage name as required.
2. The mangler works on a decl and does not have much external context. Add an implicit attribute to the decl to indicate to the mangler that a function name must be mangled.
3. Add tests for DW_AT_linkage_name for C and C++ functions.
4. Check for "sample-profile-elision-policy" attribute.

This patch addresses all the short comings pointed out in D73307 where the linkage names had to be updated explicitly and C functions could not be handled effectively.

[aaron.ballman]

clang/include/clang/Basic/Attr.td

2159

Should this attribute be inherited by redeclarations?

2165

[tmsriram]

clang/include/clang/Basic/Attr.td

2159

The attribute is added in EmitTopLevelDecl in CodeGenModule which happens after ActOnDeclarator in SemaDecl.cpp. I dont think this needs to be inherited but I am not sure. Is there an example I can use to check if this is fine?

[aaron.ballman]

clang/include/clang/Basic/Attr.td

2159

I was thinking of something along these lines, where the initial declaration is explicitly marked static and the definition is not:

static int foo(void);
int foo(void) { return 12; }

int main() {
  return foo();
}

However, I realize now that it shouldn't matter because the attribute isn't attached until codegen time. FWIW, this means the attribute won't appear in the AST, which means things like clang-tidy or third-parties that consume the AST won't have access to this information.

clang/lib/CodeGen/CodeGenModule.cpp

5561

It's a bit novel to add the attribute at codegen time rather than at sema -- is there a reason this attribute should not be added to the AST from within Sema?

[dblaikie]

clang/lib/CodeGen/CodeGenModule.cpp

5561

How would it be if this didn't use an attribute at all? If this checking was done in ItaniumMangle directly, rather than checking done here, transforming the result of the checking into an attribute, and then checking the attribute later?

[tmsriram]

clang/lib/CodeGen/CodeGenModule.cpp

5561

@dblaikie That is what I tried to do first. I didnt find an easy way to do this without either storing some state in the Mangler about every such decl or adding something to the decl itself. I guess the Mangler needs to know 3 things:

1. That funique-internal-linakge-names is enabled - maybe I can add a bool
2. That the language is not C++ - This is already possible
3. That the decl has internal linkage - this may require adding more state to the mangler

I think it is 3) that would require some plumbing unless I missed something already existing.

[tmsriram]

clang/lib/CodeGen/CodeGenModule.cpp

5561

I will revisit this, at a high level I think the mangler should know about the linkage of the decl as it does "_ZL" for this. I think I missed it.

[aaron.ballman]

clang/lib/CodeGen/CodeGenModule.cpp

5561

How would it be if this didn't use an attribute at all? If this checking was done in ItaniumMangle directly, rather than checking done here, transforming the result of the checking into an attribute, and then checking the attribute later?

I think that would have the same oddity, wouldn't it? However, I didn't do a good job of explaining my concern, which may not actually be a valid concern: would someone ever want to use this information from, say, an AST matcher for a clang-tidy check? Or is this purely a way to plumb information through the compiler without adding extra bits on a FunctionDecl? My concern was mostly that someone may want to do an AST match on this and they won't be able to because the attribute is added too late in the process.

We currently use 40 bits within FunctionDeclBitfields, so adding a single bit there does not seem like it would be the end of the world (but I didn't check thoroughly for issues with adding another bit there), so that's an option for squirreling the information away without something as heavy as an attribute.

[tmsriram]

clang/include/clang/Basic/Attr.td

2159

Ack. I will make sure I check for this case and also see if I can avoid using the attribute in some manner.

[dblaikie]

clang/lib/CodeGen/CodeGenModule.cpp

5561

the mangler has the clang::Function, so I'd have thought it could figure out the linkage from that - but I realize the linkage calculations are a bit more complicated/probably need some other sources

[tmsriram]

Removed the atttribute and directly added logic in ItaniumMangle.cpp to check if decl is internal linkage as per @dblaikie 's suggestion.

ItaniumMangle.cpp generates "_ZL" mangling for internal linkage symbols for C functions. Reused the same logic to check if a decl is internal linkage and needs a unique name. Added one extra bit to ItaniumMangle.cpp to record that -funique-internal-linkage-names is used.

[tmsriram]

Fix a bug.

[dblaikie]

clang/lib/AST/Mangle.cpp

119–124 ↗

(On Diff #324895)

Strikes me as strange to test the language mode here (presumably this logic would apply equally to C++ code, right? If at most it'd be a no-op because things are already mangled)

But, for instance - what about functions with extern "C" linkage in a C++ file?

clang/lib/CodeGen/CodeGenModule.cpp

205

Does this need the double underscore? I thought other suffixes here generally don't have that feature?

206

Might be worth a parameter name comment explaining what the 'false' parameter means? (I guess the 10 means the numeric base to use)

1176–1179

The variety of things being tested here seems non-trivial, and I've no idea how to validate it's correct. Any chance this is existing logic that could be reused from somewhere else/refactored into a common function to call from some existing use case and this new one? (similar thougts on ItaniumDemangle.cpp:isInternalLinkageDecl)

[tmsriram]

clang/lib/AST/Mangle.cpp

119–124 ↗

(On Diff #324895)

Internal linkage functions under extern "C" are already mangled even without this patch. I was surprised too. Here is the example:

extern "C" {
  static int foo(void) {
   return 0; 
  }
  static int l
  int bar() {
    return foo() + l; 
}
}

$ clang++ -c foo.cc
$ nm foo.o
T bar
t _ZL3foov
b _ZL1l

clang/lib/CodeGen/CodeGenModule.cpp

205

This was discussed in D89617 to have two underscores before uniq as the convention when I first added this identifier. Not sure if you want to revisit that.

1176–1179

So the Itanium's internal linakge decl check is exactly the logic that is used to determine when to add the "_ZL" to internal linkage names. I also refactored the original code to share the logic. For CodeGenModule.cpp, I am not sure what to use as C++ has other cases where internal linkage could be used, like anonymous namespaces maybe. I will try to dig a little more here.

[dblaikie]

clang/lib/AST/Mangle.cpp

119–124 ↗

(On Diff #324895)

ah, right, my bad.

clang/lib/CodeGen/CodeGenModule.cpp

205

Ah, thanks for the pointer! Might be worth documenting in a comment (if this is the only place that string appears)

Do you have a rough sense/list of the suffixes used by LLVM or other implementations at the moment (I see some code in LLVM that looks for ".part." and ".llvm." at least) - I don't know that "__" really provides much. The double underscore rule is to differentiate user symbols from C++ symbols in the C++ standard, but doesn't do much/anything to protect one implementation from another implementation, for instance.

1176–1179

Thanks!

[MaskRay]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

2

The test can be converted to IR test (-emit-llvm) for better separation of concerns.

Then llvm-nm & llvm-dwarfdump can be removed here.

[tmsriram]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

2

I tried that but DW_AT_linkage_name is not emitted in the IR directly. This needs to be directly checked, I can check in assembly alternately. WDYT?

[tmsriram]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

2

Oh darn it is generated as metadata, my bad. Please ignore my previous comment.

[tmsriram]

Address reviewer comments:

• Add all possible cases where internal linkage symbols can be created with C++
• Convert tests to emit LLVM IR

[dblaikie]

Looks good to me - maybe give it a week or so in case other reviewers have some unresolved concerns.

[MaskRay]

D8243

Did you link to the wrong revision?

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

5

excess spaces after -o -

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

62

If you want to test a function attribute

// UNIQUE: ... @function() [[#ATTR:]]
// UNIQUE: attributes [[#ATTR]] = {{{.*}}"sample-profile...}}

The current test does not test which definition this attribute is attached to.

[tmsriram]

Address reviewer comments:

• Fix attribute checking to check the right function
• Correct typos

[tmsriram]

In D96109#2583589, @dblaikie wrote:

Looks good to me - maybe give it a week or so in case other reviewers have some unresolved concerns.

Thanks! Regarding double-underscore for prefixes, there is also "part" which is used by function parts created via basic block sections. I dont have a strong opinion on the usage of "" and I can reopen the discussion if needed.

[MaskRay]

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

57

https://llvm.org/docs/CommandGuide/FileCheck.html#filecheck-numeric-substitution-blocks
My suggestion was [[#ATTR:]], which is shorter.

[tmsriram]

Address reviewer comment to use numeric substitution block.

[hoy]

LGTM. It addresses the C scenario smartly. Thanks.

[ellis]

Hi @tmsriram. I found a clang crash when using the flag -funique-internal-linkage-names that only happens after this diff. Would you mind taking a look?

$ cat test.c
inline void foo() {}
extern void foo();
$ clang -funique-internal-linkage-names test.c -c

More details are in this bug report https://github.com/llvm/llvm-project/issues/54139

clang/include/clang/Basic/Attr.td

let AdditionalMembers = [{
     }
   }];
   let Args = [IdentifierArgument<"Module">,
               VariadicParamIdxArgument<"Args">];
   let Subjects = SubjectList<[HasFunctionProto]>;
   let Documentation = [Undocumented];
 }
 
+def UniqueInternalLinkageSuffix : Attr {
+  // This attribute has no spellings as it is only ever created implicitly.
+  // It is used to tag function decls that are 'C' functions and have
+  // internal linkage along with the unique suffix.  Such names must be
+  // mangled and this attribute is used to denote that.
+  let Spellings = [];
+  let Subjects = SubjectList<[Function], ErrorDiag>;
+  let SemaHandler = 0;
+  let Documentation = [Undocumented];
+}
+
 def Packed : InheritableAttr {
   let Spellings = [GCC<"packed">];
 //  let Subjects = [Tag, Field];
   let Documentation = [Undocumented];
 }
 
 def IntelOclBicc : DeclOrTypeAttr {
   let Spellings = [Clang<"intel_ocl_bicc", 0>];

clang/lib/AST/ItaniumMangle.cpp

 bool ItaniumMangleContextImpl::shouldMangleCXXName(const NamedDecl *D) {
   const FunctionDecl *FD = dyn_cast<FunctionDecl>(D);
   if (FD) {
     LanguageLinkage L = FD->getLanguageLinkage();
     // Overloadable functions need mangling.
     if (FD->hasAttr<OverloadableAttr>())
       return true;
 
+    // Unique Internal Linkage functions with suffixes need mangling.
+    if (FD->hasAttr<UniqueInternalLinkageSuffixAttr>())
+      return true;
+
     // "main" is not mangled.
     if (FD->isMain())
       return false;
 
     // The Windows ABI expects that we would never mangle "typical"
     // user-defined entry points regardless of visibility or freestanding-ness.
     //
     // N.B. This is distinct from asking about "main".  "main" has a lot of

clang/lib/CodeGen/BackendUtil.cpp

 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Scalar/GVN.h"
 #include "llvm/Transforms/Scalar/LowerMatrixIntrinsics.h"
 #include "llvm/Transforms/Utils.h"
 #include "llvm/Transforms/Utils/CanonicalizeAliases.h"
 #include "llvm/Transforms/Utils/EntryExitInstrumenter.h"
 #include "llvm/Transforms/Utils/NameAnonGlobals.h"
 #include "llvm/Transforms/Utils/SymbolRewriter.h"
-#include "llvm/Transforms/Utils/UniqueInternalLinkageNames.h"
 #include <memory>
 using namespace clang;
 using namespace llvm;
 
 #define HANDLE_EXTENSION(Ext)                                                  \
   llvm::PassPluginLibraryInfo get##Ext##PluginInfo();
 #include "llvm/Support/Extension.def"
 
void EmitAssemblyHelper::CreatePasses(legacy::PassManager &MPM,
   FPM.add(new TargetLibraryInfoWrapperPass(*TLII));
   if (CodeGenOpts.VerifyModule)
     FPM.add(createVerifierPass());
 
   // Set up the per-module pass manager.
   if (!CodeGenOpts.RewriteMapFiles.empty())
     addSymbolRewriterPass(CodeGenOpts, &MPM);
 
-  // Add UniqueInternalLinkageNames Pass which renames internal linkage symbols
-  // with unique names.
-  if (CodeGenOpts.UniqueInternalLinkageNames) {
-    MPM.add(createUniqueInternalLinkageNamesPass());
-  }
-
   if (Optional<GCOVOptions> Options = getGCOVOptions(CodeGenOpts, LangOpts)) {
     MPM.add(createGCOVProfilerPass(*Options));
     if (CodeGenOpts.getDebugInfo() == codegenoptions::NoDebugInfo)
       MPM.add(createStripSymbolsPass(true));
   }
 
   if (Optional<InstrProfOptions> Options =
           getInstrProfOptions(CodeGenOpts, LangOpts))
void EmitAssemblyHelper::EmitAssemblyWithNewPassManager(
   PTO.LoopInterleaving = CodeGenOpts.UnrollLoops;
   PTO.LoopVectorization = CodeGenOpts.VectorizeLoop;
   PTO.SLPVectorization = CodeGenOpts.VectorizeSLP;
   PTO.MergeFunctions = CodeGenOpts.MergeFunctions;
   // Only enable CGProfilePass when using integrated assembler, since
   // non-integrated assemblers don't recognize .cgprofile section.
   PTO.CallGraphProfile = !CodeGenOpts.DisableIntegratedAS;
   PTO.Coroutines = LangOpts.Coroutines;
-  PTO.UniqueLinkageNames = CodeGenOpts.UniqueInternalLinkageNames;
 
   PassInstrumentationCallbacks PIC;
   StandardInstrumentations SI(CodeGenOpts.DebugPassManager);
   SI.registerCallbacks(PIC);
   PassBuilder PB(CodeGenOpts.DebugPassManager, TM.get(), PTO, PGOOpt, &PIC);
 
   // Attempt to load pass plugins and register their callbacks with PB.
   for (auto &PluginFN : CodeGenOpts.PassPlugins) {

clang/lib/CodeGen/CGCall.cpp

if (CodeGenOpts.NoPLT) {
       if (auto *Fn = dyn_cast<FunctionDecl>(TargetDecl)) {
         if (!Fn->isDefined() && !AttrOnCallSite) {
           FuncAttrs.addAttribute(llvm::Attribute::NonLazyBind);
         }
       }
     }
   }
 
+  // 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)
+        FuncAttrs.addAttribute("sample-profile-suffix-elision-policy",
+                               "selected");
+    }
+  }
+
   // Collect non-call-site function IR attributes from declaration-specific
   // information.
   if (!AttrOnCallSite) {
     if (TargetDecl && TargetDecl->hasAttr<CmseNSEntryAttr>())
       FuncAttrs.addAttribute("cmse_nonsecure_entry");
 
     // Whether tail calls are enabled.
     auto shouldDisableTailCalls = [&] {

clang/lib/CodeGen/CodeGenModule.h

 #include "clang/AST/GlobalDecl.h"
 #include "clang/AST/Mangle.h"
 #include "clang/Basic/ABI.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/Module.h"
 #include "clang/Basic/SanitizerBlacklist.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Basic/XRayLists.h"
+#include "clang/Lex/PreprocessorOptions.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/ValueHandle.h"
 #include "llvm/Transforms/Utils/SanitizerStats.h"
 

 class InitListExpr;
 class StringLiteral;
 class NamedDecl;
 class ValueDecl;
 class VarDecl;
 class LangOptions;
 class CodeGenOptions;
 class HeaderSearchOptions;
-class PreprocessorOptions;
 class DiagnosticsEngine;
 class AnnotateAttr;
 class CXXDestructorDecl;
 class Module;
 class CoverageSourceInfo;
 class TargetAttr;
 class InitSegAttr;
 struct ParsedTargetAttr;
private:
   const PreprocessorOptions &PreprocessorOpts; // Only used for debug info.
   const CodeGenOptions &CodeGenOpts;
   unsigned NumAutoVarInit = 0;
   llvm::Module &TheModule;
   DiagnosticsEngine &Diags;
   const TargetInfo &Target;
   std::unique_ptr<CGCXXABI> ABI;
   llvm::LLVMContext &VMContext;
+  std::string ModuleNameHash = "";
 
   std::unique_ptr<CodeGenTBAA> TBAA;
 
   mutable std::unique_ptr<TargetCodeGenInfo> TheTargetCodeGenInfo;
 
   // This should not be moved earlier, since its initialization depends on some
   // of the previous reference members being already initialized and also checks
   // if TheTargetCodeGenInfo is NULL
public:
   CGObjCRuntime &getObjCRuntime() {
     if (!ObjCRuntime) createObjCRuntime();
     return *ObjCRuntime;
   }
 
   /// Return true iff an Objective-C runtime has been configured.
   bool hasObjCRuntime() { return !!ObjCRuntime; }
 
+  const std::string &getModuleNameHash() const { return ModuleNameHash; }
+
   /// Return a reference to the configured OpenCL runtime.
   CGOpenCLRuntime &getOpenCLRuntime() {
     assert(OpenCLRuntime != nullptr);
     return *OpenCLRuntime;
   }
 
   /// Return a reference to the configured OpenMP runtime.
   CGOpenMPRuntime &getOpenMPRuntime() {

clang/lib/CodeGen/CodeGenModule.cpp

if (CodeGenOpts.hasProfileClangUse()) {
     } else
       PGOReader = std::move(ReaderOrErr.get());
   }
 
   // If coverage mapping generation is enabled, create the
   // CoverageMappingModuleGen object.
   if (CodeGenOpts.CoverageMapping)
     CoverageMapping.reset(new CoverageMappingModuleGen(*this, *CoverageInfo));
+
+  // Generate the module name hash here if needed.
+  if (CodeGenOpts.UniqueInternalLinkageNames &&
+      !getModule().getSourceFileName().empty()) {
+    std::string Path = getModule().getSourceFileName();
+    // Check if a path substitution is needed from the MacroPrefixMap.
+    for (const auto &Entry : PPO.MacroPrefixMap)
+      if (Path.rfind(Entry.first, 0) != std::string::npos) {
+        Path = Entry.second + Path.substr(Entry.first.size());
+        break;
+      }
+    llvm::MD5 Md5;
+    Md5.update(Path);
+    llvm::MD5::MD5Result R;
+    Md5.final(R);
+    SmallString<32> Str;
+    llvm::MD5::stringifyResult(R, Str);
+    // Convert MD5hash to Decimal. Demangler suffixes can either contain
+    // numbers or characters but not both.
+    llvm::APInt IntHash(128, Str.str(), 16);
+    // Prepend "__uniq" before the hash for tools like profilers to understand
+    // that this symbol is of internal linkage type.
+    ModuleNameHash = (Twine(".__uniq.")
+                      + Twine(IntHash.toString(10, false))).str();
+  }
 }
 
 CodeGenModule::~CodeGenModule() {}
 
 void CodeGenModule::createObjCRuntime() {
   // This is just isGNUFamily(), but we want to force implementors of
   // new ABIs to decide how best to do this.
   switch (LangOpts.ObjCRuntime.getKind()) {
static void AppendTargetMangling(const CodeGenModule &CGM,
   for (StringRef Feat : Info.Features) {
     if (!IsFirst)
       Out << '_';
     IsFirst = false;
     Out << Feat.substr(1);
   }
 }
 
-static std::string getMangledNameImpl(const CodeGenModule &CGM, GlobalDecl GD,
+// Returns true if GD is a function/var decl with internal linkage and
+// needs a unique suffix after the mangled name.
+static bool isUniqueInternalLinkageDecl(GlobalDecl GD,
+                                        CodeGenModule &CGM) {
+  const Decl *D = GD.getDecl();
+  if (!CGM.getModuleNameHash().empty() &&
+      ((isa<FunctionDecl>(D) &&
+        CGM.getFunctionLinkage(GD) == llvm::GlobalValue::InternalLinkage) ||
+       (isa<VarDecl>(D) && CGM.getContext().GetGVALinkageForVariable(
+                               cast<VarDecl>(D)) == GVA_Internal)))
+    return true;
+  return false;
+}
+
+static std::string getMangledNameImpl(CodeGenModule &CGM, GlobalDecl GD,
                                       const NamedDecl *ND,
                                       bool OmitMultiVersionMangling = false) {
   SmallString<256> Buffer;
   llvm::raw_svector_ostream Out(Buffer);
   MangleContext &MC = CGM.getCXXABI().getMangleContext();
-  if (MC.shouldMangleDeclName(ND))
+  bool ShouldMangle = MC.shouldMangleDeclName(ND);
+  if (ShouldMangle)
     MC.mangleName(GD.getWithDecl(ND), Out);
   else {
     IdentifierInfo *II = ND->getIdentifier();
     assert(II && "Attempt to mangle unnamed decl.");
     const auto *FD = dyn_cast<FunctionDecl>(ND);
 
     if (FD &&
         FD->getType()->castAs<FunctionType>()->getCallConv() == CC_X86RegCall) {
       Out << "__regcall3__" << II->getName();
     } else if (FD && FD->hasAttr<CUDAGlobalAttr>() &&
                GD.getKernelReferenceKind() == KernelReferenceKind::Stub) {
       Out << "__device_stub__" << II->getName();
     } else {
       Out << II->getName();
     }
   }
 
+  // Check if the module name hash should be appended for internal linkage
+  // symbols.   This should come before multi-version target suffixes are
+  // appended. This is to keep the name and module hash suffix of the
+  // internal linkage function together.  The unique suffix should only be
+  // added when name mangling is done to make sure that the final name can
+  // be properly demangled.  For example, for C functions without prototypes,
+  // name mangling is not done and the unique suffix should not be appeneded
+  // then.
+  if (ShouldMangle && isUniqueInternalLinkageDecl(GD, CGM)) {
+    assert(CGM.getCodeGenOpts().UniqueInternalLinkageNames &&
+           "Hash computed when not explicitly requested");
+    Out << CGM.getModuleNameHash();
+  }
+
   if (const auto *FD = dyn_cast<FunctionDecl>(ND))
     if (FD->isMultiVersion() && !OmitMultiVersionMangling) {
       switch (FD->getMultiVersionKind()) {
       case MultiVersionKind::CPUDispatch:
       case MultiVersionKind::CPUSpecific:
         AppendCPUSpecificCPUDispatchMangling(CGM,
                                              FD->getAttr<CPUSpecificAttr>(),
                                              GD.getMultiVersionIndex(), Out);
assert(OtherGD.getCanonicalDecl()
                ->isMultiVersion() &&
            "Other GD should now be a multiversioned function");
     // OtherFD is the version of this function that was mangled BEFORE
     // becoming a MultiVersion function.  It potentially needs to be updated.
     const FunctionDecl *OtherFD = OtherGD.getCanonicalDecl()
                                       .getDecl()
                                       ->getAsFunction()
                                       ->getMostRecentDecl();
     std::string OtherName = getMangledNameImpl(*this, OtherGD, OtherFD);
     // This is so that if the initial version was already the 'default'
     // version, we don't try to update it.
     if (OtherName != NonTargetName) {
       // Remove instead of erase, since others may have stored the StringRef
       // to this.
       const auto ExistingRecord = Manglings.find(NonTargetName);
       if (ExistingRecord != std::end(Manglings))
         Manglings.remove(&(*ExistingRecord));
void CodeGenModule::EmitTopLevelDecl(Decl *D) {
   if (D->isTemplated())
     return;
 
   // Consteval function shouldn't be emitted.
   if (auto *FD = dyn_cast<FunctionDecl>(D))
     if (FD->isConsteval())
       return;
 
+  // In C, functions with internal linkage which need unique suffix names need
+  // to be mangled. Add the UniqueInternalLinkageAttr to this decl so that the
+  // mangler does the right thing.  Check if the function has a prototype as
+  // the mangler will not do the right thing without one.
+  if (D->getKind() == Decl::Function && !getLangOpts().CPlusPlus &&
+      cast<FunctionDecl>(D)->getType()->getAs<FunctionProtoType>() &&
+      isUniqueInternalLinkageDecl(cast<FunctionDecl>(D), *this)) {
+      D->addAttr(UniqueInternalLinkageSuffixAttr::CreateImplicit(
+        getCXXABI().getMangleContext().getASTContext()));
+  }
+
   switch (D->getKind()) {
   case Decl::CXXConversion:
   case Decl::CXXMethod:
   case Decl::Function:
     EmitGlobal(cast<FunctionDecl>(D));
     // Always provide some coverage mapping
     // even for the functions that aren't emitted.
     AddDeferredUnusedCoverageMapping(D);

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

+// This test checks if C functions with internal linkage names are mangled
+// and the module hash suffixes attached including emitting DW_AT_linkage_name.
+//
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited -dwarf-version=4 -emit-obj -o %t.o  %s
+// RUN: llvm-nm %t.o | FileCheck %s --check-prefix=PLAIN-NM
+// RUN: llvm-dwarfdump %t.o | FileCheck %s --check-prefix=PLAIN-DWARF
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited -dwarf-version=4  -funique-internal-linkage-names -emit-obj -o %t.o %s
+// RUN: llvm-nm %t.o | FileCheck %s --check-prefix=UNIQUE-NM
+// RUN: llvm-dwarfdump %t.o | FileCheck %s --check-prefix=UNIQUE-DWARF
+
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited -dwarf-version=5 -emit-obj -o %t.o  %s
+// RUN: llvm-nm %t.o | FileCheck %s --check-prefix=PLAIN-NM
+// RUN: llvm-dwarfdump %t.o | FileCheck %s --check-prefix=PLAIN-DWARF
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited -dwarf-version=5 -funique-internal-linkage-names -emit-obj -o %t.o %s
+// RUN: llvm-nm %t.o | FileCheck %s --check-prefix=UNIQUE-NM
+// RUN: llvm-dwarfdump %t.o | FileCheck %s --check-prefix=UNIQUE-DWARF
+
+static int foo(void) {
+  return 0;
+}
+
+void baz() {
+  foo();
+}
+
+// PLAIN-NM: foo{{$}}
+// PLAIN-DWARF: DW_AT_name{{.*}}("foo")
+// PLAIN-DWARF-NOT: DW_AT_linkage_name
+// UNIQUE-NM: _ZL3foov.__uniq.{{[0-9]+}}
+// UNIQUE-DWARF-DAG: DW_AT_name{{.*}}("foo")
+// UNIQUE-DWARF-DAG: DW_AT_linkage_name{{.*}}("_ZL3foov.__uniq.{{[0-9]+}}")

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

+// This test checks if C++ functions with internal linkage names are mangled
+// and the module hash suffixes attached including emitting DW_AT_linkage_name.
+//
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited -dwarf-version=4 -emit-obj -o %t.o  %s
+// RUN: llvm-nm %t.o | FileCheck %s --check-prefix=PLAIN-NM
+// RUN: llvm-dwarfdump %t.o | FileCheck %s --check-prefix=PLAIN-DWARF
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited -dwarf-version=4  -funique-internal-linkage-names -emit-obj -o %t.o %s
+// RUN: llvm-nm %t.o | FileCheck %s --check-prefix=UNIQUE-NM
+// RUN: llvm-dwarfdump %t.o | FileCheck %s --check-prefix=UNIQUE-DWARF
+
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited -dwarf-version=5 -emit-obj -o %t.o  %s
+// RUN: llvm-nm %t.o | FileCheck %s --check-prefix=PLAIN-NM
+// RUN: llvm-dwarfdump %t.o | FileCheck %s --check-prefix=PLAIN-DWARF
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited -dwarf-version=5 -funique-internal-linkage-names -emit-obj -o %t.o %s
+// RUN: llvm-nm %t.o | FileCheck %s --check-prefix=UNIQUE-NM
+// RUN: llvm-dwarfdump %t.o | FileCheck %s --check-prefix=UNIQUE-DWARF
+
+static int foo(void) {
+  return 0;
+}
+
+void baz() {
+  foo();
+}
+
+// PLAIN-NM: _ZL3foov{{$}}
+// PLAIN-DWARF-DAG: DW_AT_name{{.*}}("foo")
+// PLAIN-DWARF-DAG: DW_AT_linkage_name{{.*}}("_ZL3foov")
+// UNIQUE-NM: _ZL3foov.__uniq.{{[0-9]+}}
+// UNIQUE-DWARF-DAG: DW_AT_name{{.*}}("foo")
+// UNIQUE-DWARF-DAG: DW_AT_linkage_name{{.*}}("_ZL3foov.__uniq.{{[0-9]+}}")

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

 // This test checks if internal linkage symbols get unique names with
 // -funique-internal-linkage-names option.
 // RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -o - < %s | FileCheck %s --check-prefix=PLAIN
-// RUN: %clang_cc1 -triple x86_64 -x c++ -O0 -S -emit-llvm -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
-// RUN: %clang_cc1 -triple x86_64 -x c++ -O1 -S -emit-llvm -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUEO1
-// RUN: %clang_cc1 -triple x86_64 -x c++ -O0 -S -emit-llvm -fexperimental-new-pass-manager -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
-// RUN: %clang_cc1 -triple x86_64 -x c++ -O1 -S -emit-llvm -fexperimental-new-pass-manager -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUEO1
+// RUN: %clang_cc1 -triple x86_64 -x c++  -S -emit-llvm -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
 
 static int glob;
 static int foo() {
   return 0;
 }
 
 int (*bar())() {
   return foo;

 // 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 i32 @_ZL4mverv()
 // PLAIN: define internal i32 @_ZL4mverv.sse4.2()
+// PLAIN-NOT: "sample-profile-suffix-elision-policy"
 // UNIQUE: @_ZL4glob.__uniq.{{[0-9]+}} = internal global
 // UNIQUE: @_ZZ8retAnonMvE5fGlob.__uniq.{{[0-9]+}} = internal global
 // UNIQUE: @_ZN12_GLOBAL__N_16anon_mE.__uniq.{{[0-9]+}} = internal global
 // UNIQUE: define internal i32 @_ZL3foov.__uniq.{{[0-9]+}}()
 // UNIQUE: define internal i32 @_ZN12_GLOBAL__N_14getMEv.__uniq.{{[0-9]+}}
-// UNIQUE: define weak_odr i32 ()* @_ZL4mverv.resolver()
+// UNIQUE: define weak_odr i32 ()* @_ZL4mverv.__uniq.{{[0-9]+}}.resolver()
 // UNIQUE: define internal i32 @_ZL4mverv.__uniq.{{[0-9]+}}()
-// UNIQUE: define internal i32 @_ZL4mverv.sse4.2.__uniq.{{[0-9]+}}
-// UNIQUEO1: define internal i32 @_ZL3foov.__uniq.{{[0-9]+}}()
-// UNIQUEO1: define weak_odr i32 ()* @_ZL4mverv.resolver()
-// UNIQUEO1: define internal i32 @_ZL4mverv.__uniq.{{[0-9]+}}()
-// UNIQUEO1: define internal i32 @_ZL4mverv.sse4.2.__uniq.{{[0-9]+}}
+// UNIQUE: define internal i32 @_ZL4mverv.__uniq.{{[0-9]+}}.sse4.2
+// UNIQUE: "sample-profile-suffix-elision-policy"