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

[Alpha Abdoulaye via Phabricator via llvm-commits]

Alpha marked 6 inline comments as done.
Alpha added inline comments.


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinkerCompileUnit.h:331
+  /// The DW_AT_APPLE_origin was added
+  bool AddedOriginObject;
+
----------------
JDevlieghere wrote:
> 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.
We set the value through `setOriginObject`, and read its value with `addedOriginObject`.
This value is used to figure out wether or not we "just" added `DW_AT_APPLE_origin` (so basically if it's a transformation from a compile unit that didn't have the attribute, to a compile unit that has one.)
This is because the addition of the new tag will lead us to update the slide value for the relocations inside that compile unit. If the compile unit already had that attribute before the dsymutil invocation, then there is no need to update the slide value.



================
Comment at: llvm/lib/TargetParser/Triple.cpp:92
 
+StringRef Triple::getArchName(ArchType Kind, SubArchType SubArch) {
+  switch (Kind) {
----------------
JDevlieghere wrote:
> Why was this moved?
For readability, as it was with the member functions and not the class functions, and more specifically to have it right next to `getArchTypeName` above with the arch names that is used as a default fallback here.


================
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;
   });
----------------
JDevlieghere wrote:
> Why do we need this cast?
Because the Reloc fields are of type `yaml::Hex64`, in order to be serialized


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

https://reviews.llvm.org/D158124