[PATCH] D20749: Introduce support for relative C++ ABI gated on LTO visibility.

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.
Herald added a subscriber: jdoerfert.

Hi @pcc , I'm working on revisiting this to see if this could help when building Fuchsia (D58321 <https://reviews.llvm.org/D58321>) and had a few questions I left inline.



================
Comment at: lib/CodeGen/CGVTables.cpp:532
+  for (auto &Comp : VTLayout.vtable_components()) {
+    if (Comp.isFunctionPointerKind())
+      Types.push_back(CGM.Int32Ty);
----------------
It seems that the condition for treating a vtable component as an i32 vs i8* is slightly different here than in `SetVTableInitializer`. `SetVTableInitializer` checks

```
Component.isFunctionPointerKind() && (I == 0 || !Components[I - 1].isFunctionPointerKind())
```

where `GetVTableType` just checks `Component.isFunctionPointerKind()`.

Should this also check if the first or last component is a function pointer, and does this check in `SetVTableInitializer` have any importance with the FIXME below it?


================
Comment at: test/CodeGenCXX/vtable-relative-abi.cpp:5
+
+// CHECK-ITANIUM: @_ZTV1S = hidden unnamed_addr constant { i8*, i8*, i32, i32 } { i8* null, i8* bitcast ({ i8*, i8* }* @_ZTI1S to i8*), i32 trunc (i64 sub (i64 ptrtoint (void (%struct.S*)* @_ZN1S2f1Ev to i64), i64 ptrtoint (i32* getelementptr inbounds ({ i8*, i8*, i32, i32 }, { i8*, i8*, i32, i32 }* @_ZTV1S, i32 0, i32 2) to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint (void (%struct.S*)* @_ZN1S2f2Ev to i64), i64 ptrtoint (i32* getelementptr inbounds ({ i8*, i8*, i32, i32 }, { i8*, i8*, i32, i32 }* @_ZTV1S, i32 0, i32 2) to i64)) to i32) }, align 8
+// CHECK-MS: @0 = private unnamed_addr constant { i8*, i32, i32 } { i8* bitcast (%rtti.CompleteObjectLocator* @"\01??_R4S@@6B@" to i8*), i32 trunc (i64 sub (i64 ptrtoint (void (%struct.S*)* @"\01?f1 at S@@UEAAXXZ" to i64), i64 ptrtoint (i32* getelementptr inbounds ({ i8*, i32, i32 }, { i8*, i32, i32 }* @0, i32 0, i32 1) to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint (void (%struct.S*)* @"\01?f2 at S@@UEAAXXZ" to i64), i64 ptrtoint (i32* getelementptr inbounds ({ i8*, i32, i32 }, { i8*, i32, i32 }* @0, i32 0, i32 1) to i64)) to i32) }, comdat($"\01??_7S@@6B@")
----------------
Should the second `getelementptr` access be `i32 0, i32 3` instead since it’s for the 4th element in the struct?

If so, I think this is due to some referencing problem with the `maybeMakeRelative` lambda. I ran into this issue when I moved `llvm::Type *VTableTy = VTable->getValueType();` into the following if block that initializes `AddrPointInt`. This also seems to occur for other test classes that contain more than one virtual function.


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

https://reviews.llvm.org/D20749