[PATCH] D77068: Fix crash on XCore on unused inline in EmitTargetMetadata

[Erich Keane via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGdc3f8913d2ad: Fix crash on XCore on unused inline in 
EmitTargetMetadata (authored by nigelp-xmos, committed by erichkeane).

Changed prior to commit:
  https://reviews.llvm.org/D77068?vs=273100=273138#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77068/new/

https://reviews.llvm.org/D77068

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/CodeGen/TargetInfo.h
  clang/test/CodeGen/xcore-unused-inline.c

Index: clang/test/CodeGen/xcore-unused-inline.c
===
--- /dev/null
+++ clang/test/CodeGen/xcore-unused-inline.c
@@ -0,0 +1,9 @@
+// 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) {}
Index: clang/lib/CodeGen/TargetInfo.h
===
--- clang/lib/CodeGen/TargetInfo.h
+++ clang/lib/CodeGen/TargetInfo.h
@@ -57,10 +57,11 @@
   virtual void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
CodeGen::CodeGenModule ) 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 ) const {}
+  /// emitTargetMetadata - Provides a convenient hook to handle extra
+  /// target-specific metadata for the given globals.
+  virtual void emitTargetMetadata(
+  CodeGen::CodeGenModule ,
+  const llvm::MapVector ) const {}
 
   /// Determines the size of struct _Unwind_Exception on this platform,
   /// in 8-bit units.  The Itanium ABI defines this as:
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -9535,11 +9535,15 @@
 
 class XCoreTargetCodeGenInfo : public TargetCodeGenInfo {
   mutable TypeStringCache TSC;
+  void emitTargetMD(const Decl *D, llvm::GlobalValue *GV,
+const CodeGen::CodeGenModule ) const;
+
 public:
   XCoreTargetCodeGenInfo(CodeGenTypes )
   : TargetCodeGenInfo(std::make_unique(CGT)) {}
-  void emitTargetMD(const Decl *D, llvm::GlobalValue *GV,
-CodeGen::CodeGenModule ) const override;
+  void emitTargetMetadata(CodeGen::CodeGenModule ,
+  const llvm::MapVector
+  ) const override;
 };
 
 } // End anonymous namespace.
@@ -9700,11 +9704,13 @@
 /// The output is tested by test/CodeGen/xcore-stringtype.c.
 ///
 static bool getTypeString(SmallStringEnc , const Decl *D,
-  CodeGen::CodeGenModule , TypeStringCache );
+  const CodeGen::CodeGenModule ,
+  TypeStringCache );
 
 /// XCore uses emitTargetMD to emit TypeString metadata for global symbols.
-void XCoreTargetCodeGenInfo::emitTargetMD(const Decl *D, llvm::GlobalValue *GV,
-  CodeGen::CodeGenModule ) const {
+void XCoreTargetCodeGenInfo::emitTargetMD(
+const Decl *D, llvm::GlobalValue *GV,
+const CodeGen::CodeGenModule ) const {
   SmallStringEnc Enc;
   if (getTypeString(Enc, D, CGM, TSC)) {
 llvm::LLVMContext  = CGM.getModule().getContext();
@@ -9716,6 +9722,21 @@
   }
 }
 
+void XCoreTargetCodeGenInfo::emitTargetMetadata(
+CodeGen::CodeGenModule ,
+const llvm::MapVector ) 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
 //===--===//
@@ -10049,7 +10070,8 @@
 }
 
 static bool getTypeString(SmallStringEnc , const Decl *D,
-  CodeGen::CodeGenModule , TypeStringCache ) {
+  const CodeGen::CodeGenModule ,
+  TypeStringCache ) {
   if (!D)
 return false;
 
Index: clang/lib/CodeGen/CodeGenModule.h

[PATCH] D77068: Fix crash on XCore on unused inline in EmitTargetMetadata

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77068/new/

https://reviews.llvm.org/D77068



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77068: Fix crash on XCore on unused inline in EmitTargetMetadata

[Nigel Perks via Phabricator via cfe-commits]

nigelp-xmos added a comment.

I do not have commit access, so if this is approved, please could it be 
committed against author "Nigel Perks" nig...@xmos.com ?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77068/new/

https://reviews.llvm.org/D77068



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77068: Fix crash on XCore on unused inline in EmitTargetMetadata

[Nigel Perks via Phabricator via cfe-commits]

nigelp-xmos updated this revision to Diff 273100.
nigelp-xmos retitled this revision from "[XCore] fix crash on unused inline in 
EmitTargetMetadata" to "Fix crash on XCore on unused inline in 
EmitTargetMetadata".
nigelp-xmos added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77068/new/

https://reviews.llvm.org/D77068

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/CodeGen/TargetInfo.h
  clang/test/CodeGen/xcore-unused-inline.c

Index: clang/test/CodeGen/xcore-unused-inline.c
===
--- /dev/null
+++ clang/test/CodeGen/xcore-unused-inline.c
@@ -0,0 +1,13 @@
+// REQUIRES: xcore-registered-target
+// RUN: %clang_cc1 -triple xcore-unknown-unknown -emit-llvm -o - %s | FileCheck %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.
+
+// CHECK: target triple = "xcore-unknown-unknown"
+
+// CHECK-NOT: dead_function
+
+inline void dead_function(void) {}
Index: clang/lib/CodeGen/TargetInfo.h
===
--- clang/lib/CodeGen/TargetInfo.h
+++ clang/lib/CodeGen/TargetInfo.h
@@ -57,10 +57,11 @@
   virtual void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
CodeGen::CodeGenModule ) 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 ) const {}
+  /// emitTargetMetadata - Provides a convenient hook to handle extra
+  /// target-specific metadata for the given globals.
+  virtual void emitTargetMetadata(
+  CodeGen::CodeGenModule ,
+  const llvm::MapVector ) const {}
 
   /// Determines the size of struct _Unwind_Exception on this platform,
   /// in 8-bit units.  The Itanium ABI defines this as:
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -9535,11 +9535,15 @@
 
 class XCoreTargetCodeGenInfo : public TargetCodeGenInfo {
   mutable TypeStringCache TSC;
+  void emitTargetMD(const Decl *D, llvm::GlobalValue *GV,
+const CodeGen::CodeGenModule ) const;
+
 public:
   XCoreTargetCodeGenInfo(CodeGenTypes )
   : TargetCodeGenInfo(std::make_unique(CGT)) {}
-  void emitTargetMD(const Decl *D, llvm::GlobalValue *GV,
-CodeGen::CodeGenModule ) const override;
+  void emitTargetMetadata(CodeGen::CodeGenModule ,
+  const llvm::MapVector
+  ) const override;
 };
 
 } // End anonymous namespace.
@@ -9700,11 +9704,13 @@
 /// The output is tested by test/CodeGen/xcore-stringtype.c.
 ///
 static bool getTypeString(SmallStringEnc , const Decl *D,
-  CodeGen::CodeGenModule , TypeStringCache );
+  const CodeGen::CodeGenModule ,
+  TypeStringCache );
 
 /// XCore uses emitTargetMD to emit TypeString metadata for global symbols.
-void XCoreTargetCodeGenInfo::emitTargetMD(const Decl *D, llvm::GlobalValue *GV,
-  CodeGen::CodeGenModule ) const {
+void XCoreTargetCodeGenInfo::emitTargetMD(
+const Decl *D, llvm::GlobalValue *GV,
+const CodeGen::CodeGenModule ) const {
   SmallStringEnc Enc;
   if (getTypeString(Enc, D, CGM, TSC)) {
 llvm::LLVMContext  = CGM.getModule().getContext();
@@ -9716,6 +9722,21 @@
   }
 }
 
+void XCoreTargetCodeGenInfo::emitTargetMetadata(
+CodeGen::CodeGenModule ,
+const llvm::MapVector ) 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
 //===--===//
@@ -10049,7 +10070,8 @@
 }
 
 static bool getTypeString(SmallStringEnc , const Decl *D,
-  CodeGen::CodeGenModule , TypeStringCache ) {
+  const