[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, this is looking pretty good!



================
Comment at: llvm/include/llvm/MC/MCObjectFileInfo.h:434
+  
+  // Swift5 Reflection Data Sections
+  MCSection *getSwift5FieldmdSection() { return Swift5FieldmdSection; }
----------------
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; }
  ...
/// \}
```


================
Comment at: llvm/include/llvm/Object/ObjectFile.h:37
 
+enum SwiftReflectionSectionType {
+  Swift_Fieldmd,
----------------
I think this fits better into the BinaryFormat directory. I would even consider adding a swift.h header there. After all that's also where the DWARF sections are defined (in Dwarf.def/Dwarf.h). 


================
Comment at: llvm/include/llvm/Object/ObjectFile.h:44
+  Swift_Reflstr,
+  Undefined
+};
----------------
maybe "Unknown = 0" ?


================
Comment at: llvm/lib/DWARFLinker/DWARFStreamer.cpp:311
+  case SwiftReflectionSectionType::Swift_Fieldmd:
+    ReflectionSection = MOFI->getSwift5FieldmdSection();
+    break;
----------------
Maybe just have a MOFI->getSwiftReflectionSection(ReflSectionType) function?


================
Comment at: llvm/lib/DWARFLinker/DWARFStreamer.cpp:328
+    break;
+  default:
+    return;
----------------
it's better to explicitly list all cases instead of having a default: label. This way we get a warning when someone adds a new case to the enum that isn't covered by the switch.


================
Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:302
 
+  Swift5FieldmdSection = Ctx->getMachOSection("__DWARF", "__swift5_fieldmd", 0,
+                                              SectionKind::getMetadata());
----------------
Only dsymutil puts these into the __DWARF section. We need to at least comment this, but if we can make this configurable, that would be even better.


================
Comment at: llvm/test/tools/dsymutil/Inputs/main.yaml:5116
+    - ''
+...
----------------
It's great to have this in YAML form. Do you think it might be possible to strip out parts of the executable that aren't strictly necessary for testing the dsymutil functionality? Maybe by running delta on the yaml file and test that the test still passes? I understand that some of the hardcoded offsets in the yaml might might make this hard.


================
Comment at: llvm/test/tools/dsymutil/reflection-dump.test:1
+RUN: rm -rf %t.dir && mkdir %t.dir && mkdir %t.dir/tmp
+RUN: cp %p/Inputs/main.yaml %t.dir
----------------
RUN: rm -rf %t.dir && mkdir -p %t.dir/tmp


================
Comment at: llvm/test/tools/dsymutil/reflection-dump.test:9
+
+RUN: dsymutil -oso-prepend-path=%t.dir %t.dir/main -o %t.dir/main.dSYM
+RUN: llvm-objdump -s %t.dir/main.dSYM/Contents/Resources/DWARF/main | Filecheck %s
----------------
Are there any error cases that we'd want to test?


================
Comment at: llvm/test/tools/dsymutil/reflection-dump.test:10
+RUN: dsymutil -oso-prepend-path=%t.dir %t.dir/main -o %t.dir/main.dSYM
+RUN: llvm-objdump -s %t.dir/main.dSYM/Contents/Resources/DWARF/main | Filecheck %s
+
----------------
Nice testcase!


================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.cpp:299
 
+static bool doReflectionSectionsExistInBinary(const DebugMap &Map,
+                                              LinkOptions &Options,
----------------
Does binaryHasSwiftReflectionSections() sound better? (not sure)


================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.cpp:303
+  // If input binary has swift5 reflection sections, we do not want to dump them
+  // out from the object files
+  if (Map.getTriple().isOSDarwin() && !Map.getBinaryPath().empty() &&
----------------
// If the input binary has swift5 reflection sections, there is no need to copy them to the .dSYM. Only copy them for binaries where the linker omitted the reflection metadata.


================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.cpp:310
+    // If ObjectEntry or Object has an error, no binary exists, therefore no
+    // reflection sections exist
+    if (!ObjectEntry) {
----------------
Nit: LLVM style always wants a `.` at the end of each sentence.


================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.cpp:454
+
+    if (!ReflectionSectionsPresentInBinary) {
+      auto OF =
----------------
maybe also factor this into a copySwiftReflectionMetadata() function?


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

https://reviews.llvm.org/D115007