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

[Shubham Sandeep Rastogi via Phabricator via llvm-commits]

rastogishubham marked 13 inline comments as done.
rastogishubham added inline comments.


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


================
Comment at: llvm/lib/DWARFLinker/DWARFStreamer.cpp:311
+  case SwiftReflectionSectionType::Swift_Fieldmd:
+    ReflectionSection = MOFI->getSwift5FieldmdSection();
+    break;
----------------
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.


================
Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:302
 
+  Swift5FieldmdSection = Ctx->getMachOSection("__DWARF", "__swift5_fieldmd", 0,
+                                              SectionKind::getMetadata());
----------------
aprantl wrote:
> 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.
The only thing I can think about doing here is making a function that sets the swift5 sections and takes a const char* for the section name, does that work?


================
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
----------------
aprantl wrote:
> RUN: rm -rf %t.dir && mkdir -p %t.dir/tmp
Thank you


================
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
----------------
aprantl wrote:
> Are there any error cases that we'd want to test?
The only error I saw was if you messed up the yaml files, then the otool output would come out wrong, not sure what else to check for. Any suggestions? I guess one thing I can test is that when the input binary does have the reflection metadata sections, then we should not emit them in the output. Not really an error test case though.


================
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
+
----------------
aprantl wrote:
> Nice testcase!
Thank you!


================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.cpp:299
 
+static bool doReflectionSectionsExistInBinary(const DebugMap &Map,
+                                              LinkOptions &Options,
----------------
aprantl wrote:
> Does binaryHasSwiftReflectionSections() sound better? (not sure)
I have always considered myself bad with names, so I am gonna go with yours :)


================
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) {
----------------
aprantl wrote:
> Nit: LLVM style always wants a `.` at the end of each sentence.
Thanks!


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

https://reviews.llvm.org/D115007