[PATCH] D125783: [llvm-debuginfo-analyzer] 08 - ELF Reader

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:261
+// file or a precompiled header file) has been moved from its original
+// location. That is the case when running regression tests.
+inline std::string createAlternativePath(StringRef To, StringRef From) {
----------------
probinson wrote:
> Should specify that `To` is required to have forward slashes but `From` is not.  Or, do a conversion on `To` as well as `From`.
The method is used only within the logical reader and always the parameter `To` corresponds to the binary file being analyzed, which already has any Windows backlashes converted for forward slashes. Moved the method to LVReader:

```
class LVReader {
  ...
protected:
  ...
  std::string createAlternativePath(StringRef From) {
    // During the reader initialization, any backslashes in 'InputFilename'
    // are converted to forward slashes.
    SmallString<128> Path;
    sys::path::append(Path, sys::path::Style::posix,
                      sys::path::parent_path(InputFilename),
                      sys::path::filename(sys::path::convert_to_slash(
                          From, sys::path::Style::windows)));
    return std::string(Path);
  }
  ...
};
```


================
Comment at: llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:154
+
+  return Error::success();
+}
----------------
probinson wrote:
> Unexpected file type is success?
Good finding. That should be an Error. For example:
```
llvm-debuginfo-analyzer resource-file.res --print=scopes
```
The `file_magic::windows_resource` is supported by LLVM but not by `llvm-debuginfo-analyzer`.
Adding an Error condition:

```
  return createStringError(errc::not_supported,
                           "Binary object format in '%s' is not supported.",
                           Filename.str().c_str());
```



================
Comment at: llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:172
+  for (LVReader *Reader : TheReaders)
+    if (options().getPrintExecute())
+      if (Error Err = Reader->doPrint())
----------------
probinson wrote:
> The `for` and first `if` can be swapped.
Nice point. Swapped.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:461
+        // This seems to be a tombstone for empty ranges.
+        if (Range.LowPC == 1 && Range.HighPC == 1)
+          continue;
----------------
probinson wrote:
> Any time lowpc == highpc it is an empty range.  Linkers might fill in 1 for code that gets stripped, or they might fill in something else.
You are correct. Changed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:1216
+    // - Symbol has the SF_Weak or
+    // - Symbol section index different of the DotTextSectionIndex.
+    LVSectionIndex SectionIndex = Section->getIndex();
----------------
probinson wrote:
> This implies that the tool can handle only linked executables?  Not relocatable objects?  maybe I knew that...  but this requirement means it won't work for relocatable files compiled with `-ffunction-sections`.
Changed.


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

https://reviews.llvm.org/D125783