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

[Shubham Sandeep Rastogi via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/MC/MCObjectFileInfo.h:239
+                            bool LargeCodeModel = false,
+                            StringRef Swift5ReflectionSegmentName = {});
   virtual ~MCObjectFileInfo();
----------------
JDevlieghere wrote:
> Should this be part of MCContext?
Sure!


================
Comment at: llvm/include/llvm/MC/MCObjectFileInfo.h:448
+  void initMachOMCObjectFileInfo(const Triple &T,
+                                 StringRef Swift5ReflectionSegmentName = {});
   void initELFMCObjectFileInfo(const Triple &T, bool Large);
----------------
JDevlieghere wrote:
> Similar as above, this seems really out of place.
I don't think this makes sense, I do not think the swift5reflectionsegmentname should be part of the Triple, so it has to be it's own argument.


================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.cpp:307
+      Options.FileType == OutputFileType::Object) {
+    BinaryHolder InputBinaryHolder(Options.VFS, false);
+
----------------
JDevlieghere wrote:
> Do we really need to create a new binary holder? Why can't we use the one used by the DWARFLinker?
Did not realize there was one, but I am not sure why 


```
bool generateDsymCompanion(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
                           const DebugMap &DM, SymbolMapTranslator &Translator,
                           MCStreamer &MS, raw_fd_ostream &OutFile) {
```

in MachoUtils.cpp creates it's own BinaryHolder?


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

https://reviews.llvm.org/D115007