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

[Vlad Tsyrklevich via Phabricator via cfe-commits]

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

I think it would be clearer to replace uses of 'member function pointer' with 'pointer to member function'; however, a google search shows that the usage of both terms is basically the same so not this might be just be my own bias coming through.



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


================
Comment at: clang/lib/CodeGen/CodeGenModule.h:1256
 
+  std::vector<const CXXRecordDecl *>
+  getMostBaseClasses(const CXXRecordDecl *RD);
----------------
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'.


================
Comment at: clang/lib/Driver/SanitizerArgs.cpp:233
+                             options::OPT_fno_sanitize_cfi_cross_dso, false);
+  if (CfiCrossDso)
+    Supported &= ~CFIMFCall;
----------------
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.


================
Comment at: clang/test/CodeGenCXX/type-metadata.cpp:281
 // ITANIUM: [[FA_ID]] = distinct !{}
 
 // MS: [[A8]] = !{i64 8, !"?AUA@@"}
----------------
Any reason not to include AF64/CF64/FAF16 here?


================
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";
----------------
s/member/pointer to member/ ?


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


https://reviews.llvm.org/D47567