[PATCH] D42501: [dsymutil] Generate Apple accelerator tables

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere marked 2 inline comments as done.
JDevlieghere added a comment.

Thanks Adrian, I'll address your comments before committing this tomorrow morning.



================
Comment at: tools/dsymutil/DwarfLinker.cpp:104
+/// a DIE offset but no actual DIE pointer.
+class AppleAccelTableStaticOffsetData : public AppleAccelTableData {
+public:
----------------
aprantl wrote:
> It seems odd to have this implementation live in DwarfLinker. I would have expected this to be part of AsmPrinter. Out of curiosity: Why is it not needed there? Is there something else that is only on opensource.apple.com and should really be on llvm.org?
I left it here as it was only used by the DwarfLinker, but happy to move it.

I'm not sure what you mean by your last two questions? The `AppleAccelTableStaticOffsetData` is the template argument for the new accelerator tables. Dsymutil generates different tables from the AsmPrinter; both have static offsets (the implementation in AsmPrinter queries the DIE for it's offset) and for types we emit two more atoms (QualifiedNameHash and whether it's a ObjCClassIsImplementation). 


================
Comment at: tools/dsymutil/DwarfLinker.cpp:1597
+      /// Offset to apply to PC addresses inside a function.
+      int64_t PCOffset = 0;
+
----------------
aprantl wrote:
> Does moving this field improve the layout somehow?
No, this seems unintentional. Actually it makes things worse :( 


================
Comment at: tools/dsymutil/dsymutil.h:43
+  /// Minimize
+  bool Minimize = false;
+
----------------
aprantl wrote:
> It looks like this option is entirely untested?
Because the value is always false actually it *is* tested, as this implies we always generate the pubnames and pubtypes section. I added it so I didn't have to remove the output, only to add it again once I implement the command line flag that will control this. When doing so I will also add a test that check the output for when the value is set to `true`.


https://reviews.llvm.org/D42501