leonardchan added a comment. > Okay, sure, if there are already offsets in bytes already in the AST-level > layout, I agree that we should be able to compute all of the byte offsets at > that level. > > How are you planning to handle alignments here? Currently alignment doesn't > affect the layout because all the entries are uniformly pointer-sized. If > you're planning to make the offset-to-top and vcall offsets 64-bit on 64-bit > architectures, you're going to either have interior padding or those offsets > might not be naturally-aligned. > > Personally, I would just use 32-bit offsets unless you really think you need > to support types with base adjustments more than ±2GB. And that has the nice > impact of making all the offsets within the v-table uniformly scaled again, > just by 4 bytes instead of the pointer size.
My initial idea was to insert padding, but it seems more straightforward and cleaner to just make the entire struct use 32 bit offsets as you said. There's probably a bigger issue if we're actually using types that require more than a 2GB adjustment, and we could perhaps add a check downt he line that prints an error if this somehow happens. I'll update D72959 <https://reviews.llvm.org/D72959> then this one. > Different topic: I'm still not sure why the places you've added TODOs here > are actually places you'd need to modify. My initial proposal had the idea of calling a separate method `CodeGenVTables::createRelativeVTableInitializer()` which was structured a lot differently from `CodeGenVTables::createVTableInitializer()`, but if I'm instead moving away from virtual methods to checking against this enum, and using an array of `i32`s instead of a struct with mixed `i32`s and `i8*`s, then I may not need this new method in the first place and can reuse a lot of the existing mechanisms in `CodeGenVTables::createVTableInitializer()`. This particular TODO though will be necessary for selecting the `Relative` enum: // TODO: Specify that we want to use the Relative VTableComponentLayout // here once we add the option for selecting it for Fuchsia. VTContext.reset(new ItaniumVTableContext(*this)); The idea I had here is to instead construct `new ItaniumVTableContext(*this, /*ComponentLayout=*/Relative))` once I add the flag for turning this on across Fuchsia. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77592/new/ https://reviews.llvm.org/D77592 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
