Fix crash on XCore on unused inline in EmitTargetMetadata

[nigelp-xmos]

Summary

EmitTargetMetadata passed to emitTargetMD a null pointer as returned from GetGlobalValue, for an unused inline function which has been removed from the module at that point.

A FIXME in CodeGenModule.cpp commented that the calling code in EmitTargetMetadata should be moved into the one target that needs it (XCore). A review comment agreed. So the calling loop has been moved into the XCore subclass. The check for null is done in that loop.

[nigelp-xmos]

Ping.

Just a polite ping to keep it alive. I hope that's all right. Not urgent at all.

[nigelp-xmos]

This patches fixes two Clang tests which are failing for XCore target on 2020-04-27 with "unexpected null value" assert failures:

CodeGen/2003-12-14-ExternInlineSupport.c
CoverageMapping/unused_names.c

[nigelp-xmos]

Adding code reviewer suggestions from git history. I would be grateful for review and/or reviewer suggestions. Many thanks.

[erichkeane]

It doesn't seem like it would be much work to move the EmitTargetMetadata function into the TargetInfo object (as a virtual function taking a reference to the collection). Essentially, have this take a reference to the collection instead of the 'D' and 'GV'.

I say that, because to me it seems that this check should likely be a part of that EmitTargetMetadata calling process (meaning, GV! = null is a precondition of this function).

[nigelp-xmos]

Thanks for the comment, I will look into that.

[nigelp-xmos]

As suggested by FIXME comment in code, and review comment, moved the EmitTargetMetadata loop into XCore target.

[nigelp-xmos]

Ping. I made the change suggested by @erichkeane .

[erichkeane]

The test needs work (check/check-not lines+ filecheck), otherwise I think this should be alright, particularly if no one else has commented in a while. I'd like to have you upload an updated test validating what you think should be happening before approving though.

clang/test/CodeGen/xcore-unused-inline.c

5

What is this test validating? It should likely have a check line of some sort.

[nigelp-xmos]

clang/test/CodeGen/xcore-unused-inline.c

5

Currently this file crashes clang with xcore target, so it was to check there is no crash. I will add a comment to explain. I can add a CHECK too. Thank you.

[nigelp-xmos]

Added explanatory comment and CHECK lines to test case, as per review comment.

Removed "[XCore]" from title as the changes are not all in XCore-only code.

[nigelp-xmos]

I do not have commit access, so if this is approved, please could it be committed against author "Nigel Perks" [e-mail] ?

[erichkeane]

I talked to others about the test, and they suggested the correct way to validate this is to make it simply a 'doesn't crash' test, so I'll remove the filecheck/check lines and submit this. The comment there seems sufficient to describe, so I'll live it in place.

clang/lib/CodeGen/CodeGenModule.h

private:
   void EmitDeclMetadata();
 
   /// Emit the Clang version as llvm.ident metadata.
   void EmitVersionIdentMetadata();
 
   /// Emit the Clang commandline as llvm.commandline metadata.
   void EmitCommandLineMetadata();
 
-  /// Emits target specific Metadata for global declarations.
-  void EmitTargetMetadata();
-
   /// Emit the module flag metadata used to pass options controlling the
   /// the backend to LLVM.
   void EmitBackendOptionsMetadata(const CodeGenOptions CodeGenOpts);
 
   /// Emits OpenCL specific Metadata e.g. OpenCL version.
   void EmitOpenCLMetadata();
 
   /// Emit the llvm.gcov metadata used to tell LLVM where to emit the .gcno and

clang/lib/CodeGen/CodeGenModule.cpp

if (CGDebugInfo *DI = getModuleDebugInfo())
     DI->finalize();
 
   if (getCodeGenOpts().EmitVersionIdentMetadata)
     EmitVersionIdentMetadata();
 
   if (!getCodeGenOpts().RecordCommandLine.empty())
     EmitCommandLineMetadata();
 
-  EmitTargetMetadata();
+  getTargetCodeGenInfo().emitTargetMetadata(*this, MangledDeclNames);
 
   EmitBackendOptionsMetadata(getCodeGenOpts());
 }
 
 void CodeGenModule::EmitOpenCLMetadata() {
   // SPIR v2.0 s2.13 - The OpenCL version used by the module is stored in the
   // opencl.ocl.version named metadata node.
   // C++ is backwards compatible with OpenCL v2.0.
llvm::NamedMDNode *CommandLineMetadata =
     TheModule.getOrInsertNamedMetadata("llvm.commandline");
   std::string CommandLine = getCodeGenOpts().RecordCommandLine;
   llvm::LLVMContext &Ctx = TheModule.getContext();
 
   llvm::Metadata *CommandLineNode[] = {llvm::MDString::get(Ctx, CommandLine)};
   CommandLineMetadata->addOperand(llvm::MDNode::get(Ctx, CommandLineNode));
 }
 
-void CodeGenModule::EmitTargetMetadata() {
-  // Warning, new MangledDeclNames may be appended within this loop.
-  // We rely on MapVector insertions adding new elements to the end
-  // of the container.
-  // FIXME: Move this loop into the one target that needs it, and only
-  // loop over those declarations for which we couldn't emit the target
-  // metadata when we emitted the declaration.
-  for (unsigned I = 0; I != MangledDeclNames.size(); ++I) {
-    auto Val = *(MangledDeclNames.begin() + I);
-    const Decl *D = Val.first.getDecl()->getMostRecentDecl();
-    llvm::GlobalValue *GV = GetGlobalValue(Val.second);
-    getTargetCodeGenInfo().emitTargetMD(D, GV, *this);
-  }
-}
-
 void CodeGenModule::EmitCoverageFile() {
   if (getCodeGenOpts().CoverageDataFile.empty() &&
       getCodeGenOpts().CoverageNotesFile.empty())
     return;
 
   llvm::NamedMDNode *CUNode = TheModule.getNamedMetadata("llvm.dbg.cu");
   if (!CUNode)
     return;

clang/lib/CodeGen/TargetInfo.h

public:
   /// getABIInfo() - Returns ABI info helper for the target.
   const ABIInfo &getABIInfo() const { return *Info; }
 
   /// setTargetAttributes - Provides a convenient hook to handle extra
   /// target-specific attributes for the given global.
   virtual void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
                                    CodeGen::CodeGenModule &M) const {}
 
-  /// emitTargetMD - Provides a convenient hook to handle extra
-  /// target-specific metadata for the given global.
-  virtual void emitTargetMD(const Decl *D, llvm::GlobalValue *GV,
-                            CodeGen::CodeGenModule &M) const {}
+  /// emitTargetMetadata - Provides a convenient hook to handle extra
+  /// target-specific metadata for the given globals.
+  virtual void emitTargetMetadata(
+      CodeGen::CodeGenModule &CGM,
+      const llvm::MapVector<GlobalDecl, StringRef> &MangledDeclNames) const {}
 
   /// Determines the size of struct _Unwind_Exception on this platform,
   /// in 8-bit units.  The Itanium ABI defines this as:
   ///   struct _Unwind_Exception {
   ///     uint64 exception_class;
   ///     _Unwind_Exception_Cleanup_Fn exception_cleanup;
   ///     uint64 private_1;
   ///     uint64 private_2;

clang/lib/CodeGen/TargetInfo.cpp

 public:
   XCoreABIInfo(CodeGen::CodeGenTypes &CGT) : DefaultABIInfo(CGT) {}
   Address EmitVAArg(CodeGenFunction &CGF, Address VAListAddr,
                     QualType Ty) const override;
 };
 
 class XCoreTargetCodeGenInfo : public TargetCodeGenInfo {
   mutable TypeStringCache TSC;
+  void emitTargetMD(const Decl *D, llvm::GlobalValue *GV,
+                    const CodeGen::CodeGenModule &M) const;
+
 public:
   XCoreTargetCodeGenInfo(CodeGenTypes &CGT)
       : TargetCodeGenInfo(std::make_unique<XCoreABIInfo>(CGT)) {}
-  void emitTargetMD(const Decl *D, llvm::GlobalValue *GV,
-                    CodeGen::CodeGenModule &M) const override;
+  void emitTargetMetadata(CodeGen::CodeGenModule &CGM,
+                          const llvm::MapVector<GlobalDecl, StringRef>
+                              &MangledDeclNames) const override;
 };
 
 } // End anonymous namespace.
 
 // TODO: this implementation is likely now redundant with the default
 // EmitVAArg.
 Address XCoreABIInfo::EmitVAArg(CodeGenFunction &CGF, Address VAListAddr,
                                 QualType Ty) const {

 /// symbols: definitions, declarations, functions & variables.
 ///
 /// The TypeString carries type, qualifier, name, size & value details.
 /// Please see 'Tools Development Guide' section 2.16.2 for format details:
 /// https://www.xmos.com/download/public/Tools-Development-Guide%28X9114A%29.pdf
 /// The output is tested by test/CodeGen/xcore-stringtype.c.
 ///
 static bool getTypeString(SmallStringEnc &Enc, const Decl *D,
-                          CodeGen::CodeGenModule &CGM, TypeStringCache &TSC);
+                          const CodeGen::CodeGenModule &CGM,
+                          TypeStringCache &TSC);
 
 /// XCore uses emitTargetMD to emit TypeString metadata for global symbols.
-void XCoreTargetCodeGenInfo::emitTargetMD(const Decl *D, llvm::GlobalValue *GV,
-                                          CodeGen::CodeGenModule &CGM) const {
+void XCoreTargetCodeGenInfo::emitTargetMD(
+    const Decl *D, llvm::GlobalValue *GV,
+    const CodeGen::CodeGenModule &CGM) const {
   SmallStringEnc Enc;
   if (getTypeString(Enc, D, CGM, TSC)) {
     llvm::LLVMContext &Ctx = CGM.getModule().getContext();
     llvm::Metadata *MDVals[] = {llvm::ConstantAsMetadata::get(GV),
                                 llvm::MDString::get(Ctx, Enc.str())};
     llvm::NamedMDNode *MD =
       CGM.getModule().getOrInsertNamedMetadata("xcore.typestrings");
     MD->addOperand(llvm::MDNode::get(Ctx, MDVals));
   }
 }
 
+void XCoreTargetCodeGenInfo::emitTargetMetadata(
+    CodeGen::CodeGenModule &CGM,
+    const llvm::MapVector<GlobalDecl, StringRef> &MangledDeclNames) const {
+  // Warning, new MangledDeclNames may be appended within this loop.
+  // We rely on MapVector insertions adding new elements to the end
+  // of the container.
+  for (unsigned I = 0; I != MangledDeclNames.size(); ++I) {
+    auto Val = *(MangledDeclNames.begin() + I);
+    llvm::GlobalValue *GV = CGM.GetGlobalValue(Val.second);
+    if (GV) {
+      const Decl *D = Val.first.getDecl()->getMostRecentDecl();
+      emitTargetMD(D, GV, CGM);
+    }
+  }
+}
 //===----------------------------------------------------------------------===//
 // SPIR ABI Implementation
 //===----------------------------------------------------------------------===//
 
 namespace {
 class SPIRTargetCodeGenInfo : public TargetCodeGenInfo {
 public:
   SPIRTargetCodeGenInfo(CodeGen::CodeGenTypes &CGT)
static bool appendType(SmallStringEnc &Enc, QualType QType,
 
   if (const FunctionType *FT = QT->getAs<FunctionType>())
     return appendFunctionType(Enc, FT, CGM, TSC);
 
   return false;
 }
 
 static bool getTypeString(SmallStringEnc &Enc, const Decl *D,
-                          CodeGen::CodeGenModule &CGM, TypeStringCache &TSC) {
+                          const CodeGen::CodeGenModule &CGM,
+                          TypeStringCache &TSC) {
   if (!D)
     return false;
 
   if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
     if (FD->getLanguageLinkage() != CLanguageLinkage)
       return false;
     return appendType(Enc, FD->getType(), CGM, TSC);
   }

clang/test/CodeGen/xcore-unused-inline.c

+// REQUIRES: xcore-registered-target
+// RUN: %clang_cc1 -triple xcore-unknown-unknown -emit-llvm -o - %s
+
+// D77068 fixes a segmentation fault and assertion failure "Unexpected null
+// Value" in the case of an unused inline function, when targeting xcore. This
+// test verifies that clang does not crash and does not produce code for such a
+// function.
+
+inline void dead_function(void) {}