[PATCH] D37371: [llvm-dwp] Add command line option "-e"

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Few comments below.



================
Comment at: test/tools/llvm-dwp/X86/dwos_list_from_exec_simple.test:6
+
+DWP from non-type-unit debug info for these two translation units:
+a.cpp:
----------------
Please specify compiler and command line used.


================
Comment at: tools/llvm-dwp/llvm-dwp.cpp:474
+static Expected<SmallVector<std::string, 16>>
+getDWOFilenames(StringRef ExecFilename) {
+  auto ErrOrObj = object::ObjectFile::createObjectFile(ExecFilename);
----------------
I would add few empty lines to this function to separate code blocks.


================
Comment at: tools/llvm-dwp/llvm-dwp.cpp:490
+    if (!DWOCompDir.empty()) {
+      SmallVector<char, 64> DWOPath;
+      DWOPath.reserve(DWOCompDir.size() + DWOName.size() + 1);
----------------
I think `SmallString<16>` would be more common.


================
Comment at: tools/llvm-dwp/llvm-dwp.cpp:491
+      SmallVector<char, 64> DWOPath;
+      DWOPath.reserve(DWOCompDir.size() + DWOName.size() + 1);
+      sys::path::append(DWOPath, DWOCompDir, DWOName);
----------------
I believe it is excessive to reserve here.


================
Comment at: tools/llvm-dwp/llvm-dwp.cpp:493
+      sys::path::append(DWOPath, DWOCompDir, DWOName);
+      DWOPaths.emplace_back(DWOPath.data());
+    } else {
----------------
Here is a bug. DWOPatch.data() will return pointer to internal buffer, but not a string. 
It can be not nul-terminated.

You may use following to fix probably:
```
DWOPaths.emplace_back(DWOPath.data(), DWOPath.size());
```


================
Comment at: tools/llvm-dwp/llvm-dwp.cpp:717
+    auto DWOs = getDWOFilenames(ExecFilename);
+    if (auto Error = DWOs.takeError()) {
+      logAllUnhandledErrors(std::move(Error), errs(), "error: ");
----------------
It seems this will fail if you run tests with `LLVM_ENABLE_ABI_BREAKING_CHECKS`.
You need to check `Expected<>`:
```
auto DWOs = getDWOFilenames(ExecFilename);
if (!DWOs) {
...
```
 

Otherwise if should fail:
https://github.com/llvm-mirror/llvm/blob/master/include/llvm/Support/Error.h#L507
https://github.com/llvm-mirror/llvm/blob/master/include/llvm/Support/Error.h#L636.


Repository:
  rL LLVM

https://reviews.llvm.org/D37371