[PATCH] D115007: Dump swift5 reflection section data into dsym bundle generated binary with dsymutil

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

Thanks for the update! We're down to cosmetic changes.



================
Comment at: llvm/include/llvm/BinaryFormat/Swift.def:26
+HANDLE_SWIFT_SECTION(Typeref, "__swift5_typeref", "swift5_typeref", ".sw5tyrf")
+HANDLE_SWIFT_SECTION(Reflstr, "__swift5_reflstr", "swift5_reflstr", ".sw5rfst")
----------------
This is nice.


================
Comment at: llvm/include/llvm/BinaryFormat/Swift.def:27
+HANDLE_SWIFT_SECTION(Reflstr, "__swift5_reflstr", "swift5_reflstr", ".sw5rfst")
\ No newline at end of file

----------------
missing newline


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFStreamer.h:91
+  void emitSwiftReflectionSection(
+      llvm::swift::Swift5ReflectionSectionType ReflSectionType,
+      StringRef Buffer, uint32_t Alignment, uint32_t Size);
----------------
s/type/kind/


================
Comment at: llvm/include/llvm/MC/TargetRegistry.h:394
+      MCContext &Ctx, bool PIC, bool LargeCodeModel = false,
+      StringRef *Swift5ReflectionSegmentName = nullptr) const {
     if (!MCObjectFileInfoCtorFn) {
----------------
Instead of passing a StringRef as a pointer, we can just pass a StringRef value, like this:

```
StringRef Swift5ReflectionSegmentName = {}
```

and then check for `.empty()` instead for `nullptr`.


================
Comment at: llvm/lib/DWARFLinker/DWARFStreamer.cpp:311
+  case SwiftReflectionSectionType::Swift_Fieldmd:
+    ReflectionSection = MOFI->getSwift5FieldmdSection();
+    break;
----------------
rastogishubham wrote:
> aprantl wrote:
> > Maybe just have a MOFI->getSwiftReflectionSection(ReflSectionType) function?
> I think that just moves the switch statement into a different function, and there is no overt benefit to doing that.
What's potentially more elegant is to have a std::array of reflection sections in MOFI instead of 6 separate member variables. If you change Unknown to -1 and let Fieldmd be 0 you could access it as `Swift5ReflectionSection[Swift5ReflectionSectionType::Fieldmd]`. Then we could replace the 6 accessor functions with one `getSwiftReflectionSection(Swift5ReflectionSectionType)` 


================
Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:52
+void MCObjectFileInfo::initMachOMCObjectFileInfo(
+    const Triple &T, StringRef *Swift5ReflectionSegmentName) {
   // MachO
----------------
just `StringRef`


================
Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:312
+#include "llvm/BinaryFormat/Swift.def"
+#undef HANDLE_SWIFT_SECTION
+  }
----------------
nice!


================
Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:312
+#include "llvm/BinaryFormat/Swift.def"
+#undef HANDLE_SWIFT_SECTION
+  }
----------------
aprantl wrote:
> nice!
is this redundandant?


================
Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:1001
+    MCContext &MCCtx, bool PIC, bool LargeCodeModel,
+    StringRef *Swift5ReflectionSegmentName) {
   PositionIndependent = PIC;
----------------
StringRef


================
Comment at: llvm/test/tools/dsymutil/reflection-dump.test:42
+CHECK-NEXT:  10000e244 00000000 09000000 08000100 10000000  ................
+CHECK-NEXT:  10000e254 fe000000                             ....
----------------
newline


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

https://reviews.llvm.org/D115007