[PATCH] D158124: [dsymutil] Add support for mergeable libraries

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added inline comments.


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinkerCompileUnit.h:331
+  /// The DW_AT_APPLE_origin was added
+  bool AddedOriginObject;
+
----------------
Do we only need to know if the attribute is set? Do we never need its value? If we do, why not store that?

`HasOrigin` would probably be a better name.


================
Comment at: llvm/include/llvm/DWARFLinkerParallel/AddressesMap.h:59
 
+  // Reeturns the file name associated to the AddessesMap
+  virtual std::optional<StringRef> getFileName() = 0;
----------------
Period. Also applies to line 70 below.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1693
+  case dwarf::DW_AT_APPLE_origin:
+    // Always recreate a new DW_AT_APPLE_origin attribute
+    return true;
----------------
Nit: "Always recreate the DW_AT_APPLE_origin attribute" or "Always create a new DW_AT_APPLE_origin attribute".


================
Comment at: llvm/lib/TargetParser/Triple.cpp:92
 
+StringRef Triple::getArchName(ArchType Kind, SubArchType SubArch) {
+  switch (Kind) {
----------------
Why was this moved?


================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.cpp:999
   auto CurReloc = partition_point(Relocs, [StartPos](const ValidReloc &Reloc) {
-    return Reloc.Offset < StartPos;
+    return (uint64_t)Reloc.Offset < StartPos;
   });
----------------
Why do we need this cast?


================
Comment at: llvm/tools/dsymutil/Options.td:209-218
+def build_variant_suffix: Separate<["--", "-"], "build-variant-suffix">,
+  MetaVarName<"<suffix=buildvariant>">,
+  HelpText<"Specify the build variant suffix used to build the executabe file.">,
+  Group<grp_general>;
+def: Joined<["--", "-"], "build-variant-suffix=">, Alias<build_variant_suffix>;
+
+def dsym_search_path: Separate<["-", "--"], "D">,
----------------
Please update `cmdline.test` and `dsymutil.rst`. 


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

https://reviews.llvm.org/D158124