[Lldb-commits] [PATCH] D66791: [lldb][ELF] Read symbols from .gnu_debugdata sect.

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk marked 5 inline comments as done.
kwk added inline comments.


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:1849-1851
+  if (m_gnuDebugDataObjectFile != nullptr) {
+    return m_gnuDebugDataObjectFile;
+  }
----------------
labath wrote:
> Not a big deal, but we usually don't put braces around short blocks like this. If the block spans multiple lines, then the braces are fine, even if the block technically consists of a single statement only.
Done in ab313383a79


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:1901
+  ArchSpec spec = m_gnuDebugDataObjectFile->GetArchitecture();
+  if (spec && m_gnuDebugDataObjectFile->SetModulesArchitecture(spec)) {
+    return m_gnuDebugDataObjectFile;
----------------
labath wrote:
> Are you sure you want to do that? Presumably, the architecture of the module was already set by the containing object file, and that file probably has a more correct understanding of what the right architecture is...
Correct, the containing object file has a better understanding of things. Are you suggesting to do this then?

```
ArchSpec spec = GetArchitecture();
  if (spec && m_gnuDebugDataObjectFile->SetModulesArchitecture(spec))
    return m_gnuDebugDataObjectFile;
```


================
Comment at: lldb/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp:79
+  // If there's none in the orignal object file, we add it to it.
+  if (auto gdd_obj_file =
+          obj_file->GetGnuDebugDataObjectFile()) {
----------------
labath wrote:
> kwk wrote:
> > labath wrote:
> > > Wouldn't it be better to first check for the external file, and then fall back to gnu_debugdata, as the external file will likely have more complete debug info?
> > My idea was to load whatever symbols we can get and let it be overwritten by the the more concrete ones that might come later. Changing the logic requires to your suggesttion would require a bit more effort in that I cannot simply leave the `return nullptr` expressions untouched. 
> Aha, interesting. I missed the fact that you are continuing the search after extracting the debugdata section. While I don't expect that will be too useful, it does sound like a good idea in principle.
> 
> The thing that worries me in this case is precisely the `return nullptr`. That value is supposed to mean that the symbol vendor hasn't done anything, and lldb should continue the search. But in this case, you actually *have* done something. If that's how you want to implement this, then I think it would be better if this logic was fully inside ObjectFileELF. I think you should be able to achieve that by just moving this bit of code into ObjectFileELF::CreateSections. That way, the contents of gnu_debugdata would be considered an indivisible part of the main object file (which is kind of true), and anything that the symbol vendor finds in the external files (which is his main goal) would come on top of that.
Fixed in f509e547ab793068e8b822317b93060077623b74


================
Comment at: llvm/CMakeLists.txt:53-361
+include(CMakeDependentOption)
+
 if (NOT CMAKE_BUILD_TYPE AND NOT CMAKE_CONFIGURATION_TYPES)
   message(STATUS "No build type selected, default to Debug")
   set(CMAKE_BUILD_TYPE "Debug" CACHE STRING "Build type (default Debug)" FORCE)
 endif()
 
----------------
labath wrote:
> Move this stuff into lldb's cmake file (lldb/cmake/modules/LLDBConfig.cmake, probably).
Fixed in d4236447e75a882ab61bd369cb8253f2a908368f


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66791