[PATCH] D56346: [dsymutil] Update unobfuscation logic.

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: llvm/test/tools/dsymutil/ARM/obfuscated.test:5
+RUN: dsymutil --symbol-map %p/../Inputs/obfuscated.map %p/../Inputs/obfuscated.arm64 -f -o - | llvm-dwarfdump -v - | FileCheck --check-prefix=NOHIDDEN %s
+RUN: dsymutil --symbol-map %p/../Inputs/obfuscated.2.map %p/../Inputs/obfuscated.2.arm64 -f -o - | llvm-dwarfdump -v - | FileCheck --check-prefix=NOHIDDEN %s
+
----------------
nit: how about splitting those lines at 80 chars?
```
RUN: ... \
RUN:    -...
```


================
Comment at: llvm/tools/dsymutil/DwarfStreamer.cpp:581
+/// Copy the debug_line over to the updated binary while unobfuscating the file
+/// names and directories.
+void DwarfStreamer::translateLineTable(DataExtractor Data, uint32_t Offset,
----------------
comment should be on the declaration not the implementation, I believe.


================
Comment at: llvm/tools/dsymutil/DwarfStreamer.cpp:597
+  if (Version > 5) {
+    warn("Unsupported line table version: dropping contents and not "
+         "unobfsucating line table.");
----------------
Any plans to implement this soon?


================
Comment at: llvm/tools/dsymutil/SymbolMap.cpp:50
+
+  // Objective C symbols for the Macho symbol table start with that weird \1
+  // character. Do not preprend an underscore to these and drop that initial
----------------
s/that weird/cross reference to the MC code that explains why we do this/ ?


================
Comment at: llvm/tools/dsymutil/SymbolMap.cpp:54
+  if (Translation[0] == 1)
+    return llvm::StringRef(Translation).drop_front();
+
----------------
this is in namespace llvm, does this need to be qualified?


================
Comment at: llvm/tools/dsymutil/SymbolMap.cpp:68
+  std::string SymbolMapPath = SymbolMapFile;
+#if __APPLE__
+  // Look through the UUID Map
----------------
It's unfortunate that dsymutil on non-macOS will behave differently... but this is not a regression


================
Comment at: llvm/tools/dsymutil/SymbolMap.cpp:69
+#if __APPLE__
+  // Look through the UUID Map
+  if (llvm::sys::fs::is_directory(SymbolMapPath) && !Map.getUUID().empty()) {
----------------
`.`


================
Comment at: llvm/tools/dsymutil/SymbolMap.cpp:74
+    std::string plistFilename = (llvm::sys::path::parent_path(InputFile) +
+                                 llvm::Twine("/../") + UUIDString + ".plist")
+                                    .str();
----------------
why parent_path first and then `..`?


================
Comment at: llvm/tools/dsymutil/SymbolMap.cpp:93
+              plist, CFSTR("DBGOriginalUUID"));
+          SymbolMapPath += (llvm::Twine("/") +
+                            llvm::StringRef(CFStringGetCStringPtr(
----------------
perhaps introduce a local var to make this more readable with 80 chars?


================
Comment at: llvm/tools/dsymutil/SymbolMap.cpp:133
+  bool MangleNames = false;
+  if (!LHS.startswith("BCSymbolMap Version:")) {
+    // Version string not present, warns but try to parse it.
----------------
perhaps use StringRef::consume() for premature optimization?


================
Comment at: llvm/tools/dsymutil/SymbolMap.h:22
+
+class SymbolMapTranslator {
+public:
----------------
Doxygen comment?


================
Comment at: llvm/tools/dsymutil/SymbolMap.h:26
+
+  SymbolMapTranslator(std::vector<std::string> UnobfuscatedStrings,
+                      bool MangleNames)
----------------
ArrayRef or && or const& perhaps?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56346