[PATCH] D105909: [clang][CallGraphSection] Add type id metadata to indirect call and targets

[Necip Fazil Yildiran via Phabricator via cfe-commits]

necipfazil added inline comments.


================
Comment at: clang/lib/CodeGen/CGObjCMac.cpp:2288
+    }
+  }
 
----------------
morehouse wrote:
> This is for Objective C?  I don't think we care about Objective C.
I will remove this.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1878
+       CodeGenOpts.SanitizeCfiCanonicalJumpTables) ||
+      CodeGenOpts.CallGraphSection) {
     if (auto *FD = dyn_cast<FunctionDecl>(D)) {
----------------
morehouse wrote:
> Why do we need the type metadata on function definitions?
Actually, we don’t.  It seems like having this may lead to duplicate metadata as well.

I will remove this.


================
Comment at: clang/test/CodeGen/call-graph-section.c:39
+  fp_qux(0, 0, 0);
+}
+
----------------
morehouse wrote:
> Should we also add a test with a C struct parameter?
I will add one.


================
Comment at: clang/test/CodeGen/call-graph-section.c:41
+
+// ITANIUM-DAG: [[TVOID_GENERALIZED]] = !{{{.*}}!"_ZTSFvE.generalized"}
+// ITANIUM-DAG: [[TPRIMITIVE_GENERALIZED]] = !{{{.*}}!"_ZTSFicfdE.generalized"}
----------------
morehouse wrote:
> What is the `{{.*}}` consuming here?
Internally, for functions, this metadata is added using `llvm::GlobalObject::addTypeMetadata()`, which takes an offset parameter. This is always 0 for our case. `{{.*}}` is consuming `i64 0,` if the metadata belongs to a function. Otherwise, for callsites, it consumes nothing.

I will make the distinction in the test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105909