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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
