[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 reducing the test! This looks much more palatable.



================
Comment at: llvm/include/llvm/BinaryFormat/Swift.h:14
+
+enum Swift5ReflectionSectionType {
+  Unknown = 0,
----------------
If it's already in the namespace we don't need to repeat Swift in the name. I would go with:
```
enum Swift5ReflectionSectionKind {
  Unknown = 0,
  Fieldmd,
  ...
};

or 

enum ReflectionSectionKind {
  Unknown = 0,
  Swift5Fieldmd,
  ...
};
```


================
Comment at: llvm/include/llvm/BinaryFormat/Swift.h:21
+  Swift_Typeref,
+  Swift_Reflstr
+};
----------------
We might want to split this out into a .def file. See Dwarf.def for an example:

Swift.def:
```
#ifdef HANDLE_SWIFT_SECTION
#undef HANDLE_SWIFT_SECTION
#endif
// Enum Kind, Mach-O name, ELF name, COFF name.
HANDLE_SWIFT_SECTION(Swift5Fieldmd, "__swift5fieldmd", ".swift5fieldmd", ???)
```
Swift.h
```
enum ReflectionSectionKind {
  Unknown = 0,
#define HANDLE_SWIFT_SECTION(KIND, MACHO, ELF, COFF) #KIND,
#include "llvm/BinaryFormat/Swift/def"
};
```


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFStreamer.h:90
+  /// Emit the swift5 reflection section stored in \p Buffer.
+  void emitSwift5ReflectionSection(
+      llvm::swift::Swift5ReflectionSectionType ReflSectionType,
----------------
I would drop the "5" here.


================
Comment at: llvm/include/llvm/MC/MCObjectFileInfo.h:231
 
+  // Swift5 Reflection Data Sections
+  MCSection *Swift5FieldmdSection = nullptr;
----------------
Here the Swift"5" makes sense.


================
Comment at: llvm/include/llvm/MC/MCObjectFileInfo.h:450
+  void initMachOMCObjectFileInfo(const Triple &T,
+                                 const char *Swift5ReflectionSegmentName);
   void initELFMCObjectFileInfo(const Triple &T, bool Large);
----------------
this should be a StringRef


================
Comment at: llvm/include/llvm/MC/MCObjectFileInfo.h:434
+  
+  // Swift5 Reflection Data Sections
+  MCSection *getSwift5FieldmdSection() { return Swift5FieldmdSection; }
----------------
rastogishubham wrote:
> aprantl wrote:
> > I know the other comments in this file also don't use this format, but FYI, you can doxygenify this as:
> > ```
> > /// Swift5 Reflection Data Sections.
> > /// \{
> > MCSection *getSwift5FieldmdSection() { return Swift5FieldmdSection; }
> >   MCSection *getSwift5AssoctySection() { return Swift5AssoctySection; }
> >   MCSection *getSwift5BuiltinSection() { return Swift5BuiltinSection; }
> >   ...
> > /// \}
> > ```
> Good to know, would it be weird to doxygen just the functions we added though?
I would do a second NFC patch to just doxygenify the entire class.


================
Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:303
 
+  // To dump out swift5 reflection metadata, we need to dump it to a particular
+  // segment. At the current time, the way dsymutil is written, dumping the data
----------------
I would just write:
```
// The architecture of dsymutil makes it very difficult to copy the Swift reflection metadata sections into the __TEXT segment, so dsymutil creates these sections in the __DWARF segment instead.
```


================
Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:311
+      Ctx->getMachOSection(Swift5ReflectionSegmentName, "__swift5_fieldmd", 0,
+                           SectionKind::getMetadata());
+
----------------
If you create a Swift.def file, this can also be just a single #include with some clever macro substitutions. Check out the dwarf names are stringified for an example.


================
Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:1042
   case MCContext::IsMachO:
-    initMachOMCObjectFileInfo(TheTriple);
+    initMachOMCObjectFileInfo(TheTriple, "__DWARF");
     break;
----------------
The DWARF segment needs to be hardcoded in dsymutil or DwarfLinker. The MC layer should not know about this. So we need to thread the flag all the way to DwarfLinker.


================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:4725
+llvm::swift::Swift5ReflectionSectionType
+MachOObjectFile::mapReflectionSectionNameToEnumValue(
+    StringRef SectionName) const {
----------------
If you create a Swift.def file, this can also be just a single #include with some clever macro substitutions. Check out the dwarf names are stringified for an example.


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

https://reviews.llvm.org/D115007