[PATCH] D58321: Support for relative vtables

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added inline comments.


================
Comment at: clang/include/clang/AST/DeclCXX.h:531
+    /// \brief Whether the class uses the relative C++ vtable ABI.
+    unsigned IsRelativeCXXABI : 1;
+
----------------
pcc wrote:
> rjmccall wrote:
> > Should we proactively generalize this as a "CXXABIVariant" enum, which for now can just be "Standard" and "RelativeVTables"?
> > 
> > Also, I don't want to pre-empt your secret plans, but if Fuchsia is just going to use this as its system C++ ABI, maybe we should plan for that, too.
> At this point I probably would remove this bitfield entirely. This implementation does not support enabling the ABI on a per-class basis, so everywhere that we are currently checking this field we should just be able to check `RelativeCXXABIVTables` in `LangOptions`.
The goal for us is to just enable this for all vtables so we don't need this on a per-class basis. Removed the bitfield.


================
Comment at: clang/include/clang/Basic/LangOptions.def:329
+        "Whether to use clang's relative C++ ABI "
+        "for classes with vtables")
+
----------------
rjmccall wrote:
> Yeah, see, this plays into the question above.  I would not want to provide this as a language option for general use.  The attribute seems good enough for testing, and if you want a -cc1 option to apply the attribute by default for experimentation during Fuchsia bring-up that's fair, but I don't want something that suggests to users that it's okay to pass this attribute and change the system default.
Ok. So is this the reason for the white list approach mentioned in D17893? As an alternative, would you also be ok with creating a `FuchsiaCXXABI` that subclasses `ItaniumCXXABI`, similar to the ARM and WebAssembly ones? This way it doesn't change the system default.


================
Comment at: clang/include/clang/Sema/Sema.h:10976
+  /// Determine if this class can use the relative vtable ABI.
+  void checkClassABI(CXXRecordDecl *RD);
+
----------------
rjmccall wrote:
> Comment / method-name mismatch?
Removed this method since the bitfield we set here is removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58321