[PATCH] D47567: Implement CFI for indirect calls via a member function pointer.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1413
+              FD->getType(), Context.getRecordType(Base).getTypePtr()));
+      F->addTypeMetadata(0, Id);
+    }
----------------
vlad.tsyrklevich wrote:
> It'd be nice to have a test that reaches this.
I'll add one.


================
Comment at: clang/lib/CodeGen/CodeGenModule.h:1256
 
+  std::vector<const CXXRecordDecl *>
+  getMostBaseClasses(const CXXRecordDecl *RD);
----------------
vlad.tsyrklevich wrote:
> Could be helpful to have a comment here to ensure there is no confusion interpreting this as 'the most-base classes' and not 'most of the base classes'.
Will do.


================
Comment at: clang/lib/Driver/SanitizerArgs.cpp:233
+                             options::OPT_fno_sanitize_cfi_cross_dso, false);
+  if (CfiCrossDso)
+    Supported &= ~CFIMFCall;
----------------
vlad.tsyrklevich wrote:
> This will cause supplying both options to fail with `clang: error: unsupported option '-fsanitize=cfi-mfcall' for target ...`. Having it error out the same way as type generalization below where it states that cfi-cross-dso is unsupported with cfi-mfcall seems like a more helpful error.
The issue with that is that it would cause the flag combination `-fsanitize=cfi -fsanitize-cfi-cross-dso` to fail with an unsupported error. So I think it would need to work in a similar way as with the supported sanitizers. I guess I could add something after line 293 below instead.


================
Comment at: clang/test/CodeGenCXX/type-metadata.cpp:281
 // ITANIUM: [[FA_ID]] = distinct !{}
 
 // MS: [[A8]] = !{i64 8, !"?AUA@@"}
----------------
vlad.tsyrklevich wrote:
> Any reason not to include AF64/CF64/FAF16 here?
No, I just forgot to add them. I'll do that.


================
Comment at: compiler-rt/lib/ubsan/ubsan_handlers.cc:645
+  const char *CheckKindStr = Data->CheckKind == CFITCK_NVMFCall
+                                 ? "non-virtual member function call"
+                                 : "indirect function call";
----------------
vlad.tsyrklevich wrote:
> s/member/pointer to member/ ?
Makes sense.


================
Comment at: compiler-rt/lib/ubsan/ubsan_handlers_cxx.cc:126
+  case CFITCK_VMFCall:
+    CheckKindStr = "virtual member function call";
+    break;
----------------
vlad.tsyrklevich wrote:
> s/member/pointer to member/ ?
Ditto


https://reviews.llvm.org/D47567