[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi marked an inline comment as done.
EugeneBi added inline comments.



Comment at: 
packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/TestLinuxCore.py:215
+# /home/labath/test/a.out)
+tmp_sysroot = "/tmp/lldb_i386_mock_sysroot"
+executable = tmp_sysroot + "/home/labath/test/a.out"

labath wrote:
> EugeneBi wrote:
> > labath wrote:
> > > Please put this in the build folder (self.getBuildDir) instead of `/tmp`.
> > I uploaded new diff and closed revision. 
> > Do I need to do anything else?
> Normally, the revision would be automatically closed when the patch is 
> committed. Do you have commit access or you need someone to commit this for 
> you?
I do not have commit access.
Could you commit it, please?


https://reviews.llvm.org/D49685



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi marked an inline comment as done.
EugeneBi added inline comments.



Comment at: 
packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/TestLinuxCore.py:215
+# /home/labath/test/a.out)
+tmp_sysroot = "/tmp/lldb_i386_mock_sysroot"
+executable = tmp_sysroot + "/home/labath/test/a.out"

labath wrote:
> Please put this in the build folder (self.getBuildDir) instead of `/tmp`.
I uploaded new diff and closed revision. 
Do I need to do anything else?


https://reviews.llvm.org/D49685



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi updated this revision to Diff 161959.
EugeneBi added a comment.

Do not use /tmp directory in the test


https://reviews.llvm.org/D49685

Files:
  
packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/TestLinuxCore.py
  source/Target/Platform.cpp

Index: source/Target/Platform.cpp
===
--- source/Target/Platform.cpp
+++ source/Target/Platform.cpp
@@ -228,17 +228,36 @@
 module_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr,
 did_create_ptr, false);
 
+  // Module resolver lambda.
+  auto resolver = [&](const ModuleSpec ) {
+Status error(eErrorTypeGeneric);
+ModuleSpec resolved_spec;
+// Check if we have sysroot set.
+if (m_sdk_sysroot) {
+  // Prepend sysroot to module spec.
+  resolved_spec = spec;
+  resolved_spec.GetFileSpec().PrependPathComponent(
+m_sdk_sysroot.GetStringRef());
+  // Try to get shared module with resolved spec.
+  error = ModuleList::GetSharedModule( 
+resolved_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr, 
+did_create_ptr, false);
+}
+// If we don't have sysroot or it didn't work then 
+// try original module spec.
+if (!error.Success()) {
+  resolved_spec = spec;
+  error = ModuleList::GetSharedModule(
+resolved_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr, 
+did_create_ptr, false);
+}
+if (error.Success() && module_sp)
+  module_sp->SetPlatformFileSpec(resolved_spec.GetFileSpec());
+return error;
+  };
+
   return GetRemoteSharedModule(module_spec, process, module_sp,
-   [&](const ModuleSpec ) {
- Status error = ModuleList::GetSharedModule(
- spec, module_sp, module_search_paths_ptr,
- old_module_sp_ptr, did_create_ptr, false);
- if (error.Success() && module_sp)
-   module_sp->SetPlatformFileSpec(
-   spec.GetFileSpec());
- return error;
-   },
-   did_create_ptr);
+   resolver, did_create_ptr);
 }
 
 bool Platform::GetModuleSpec(const FileSpec _file_spec,
Index: packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/TestLinuxCore.py
===
--- packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -6,6 +6,7 @@
 
 import shutil
 import struct
+import os
 
 import lldb
 from lldbsuite.test.decorators import *
@@ -203,6 +204,30 @@
 for regname, value in values.iteritems():
 self.expect("register read {}".format(regname), substrs=["{} = {}".format(regname, value)])
 
+@expectedFailureAll(bugnumber="llvm.org/pr37371", hostoslist=["windows"])
+@skipIf(triple='^mips')
+@skipIfLLVMTargetMissing("X86")
+def test_i386_sysroot(self):
+"""Test that lldb can find the exe for an i386 linux core file using the sysroot."""
+
+# Copy linux-i386.out to tmp_sysroot/home/labath/test/a.out (since it was compiled as
+# /home/labath/test/a.out)
+tmp_sysroot = os.path.join(self.getBuildDir(), "lldb_i386_mock_sysroot")
+executable = os.path.join(tmp_sysroot, "home", "labath", "test", "a.out")
+lldbutil.mkdir_p(os.path.dirname(executable))
+shutil.copyfile("linux-i386.out", executable)
+
+# Set sysroot and load core
+self.runCmd("platform select remote-linux --sysroot '%s'" % tmp_sysroot)
+target = self.dbg.CreateTarget(None)
+self.assertTrue(target, VALID_TARGET)
+process = target.LoadCore("linux-i386.core")
+
+# Check that we found a.out from the sysroot
+self.check_all(process, self._i386_pid, self._i386_regions, "a.out")
+
+self.dbg.DeleteTarget(target)
+
 def check_memory_regions(self, process, region_count):
 region_list = process.GetMemoryRegions()
 self.assertEqual(region_list.GetSize(), region_count)
@@ -299,15 +324,7 @@
 self.dbg.SetOutputFileHandle(None, False)
 self.dbg.SetErrorFileHandle(None, False)
 
-def do_test(self, filename, pid, region_count, thread_name):
-target = self.dbg.CreateTarget(filename + ".out")
-process = target.LoadCore(filename + ".core")
-self.assertTrue(process, PROCESS_IS_VALID)
-self.assertEqual(process.GetNumThreads(), 1)
-self.assertEqual(process.GetProcessID(), pid)
-
-self.check_state(process)
-
+def check_stack(self, process, pid, thread_name):
 thread = process.GetSelectedThread()
 self.assertTrue(thread)
   

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi updated this revision to Diff 161520.
EugeneBi added a comment.

Mark added the test. Please let us know if this is OK.


https://reviews.llvm.org/D49685

Files:
  
packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/TestLinuxCore.py
  source/Target/Platform.cpp

Index: source/Target/Platform.cpp
===
--- source/Target/Platform.cpp
+++ source/Target/Platform.cpp
@@ -228,17 +228,36 @@
 module_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr,
 did_create_ptr, false);
 
+  // Module resolver lambda.
+  auto resolver = [&](const ModuleSpec ) {
+Status error(eErrorTypeGeneric);
+ModuleSpec resolved_spec;
+// Check if we have sysroot set.
+if (m_sdk_sysroot) {
+  // Prepend sysroot to module spec.
+  resolved_spec = spec;
+  resolved_spec.GetFileSpec().PrependPathComponent(
+m_sdk_sysroot.GetStringRef());
+  // Try to get shared module with resolved spec.
+  error = ModuleList::GetSharedModule( 
+resolved_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr, 
+did_create_ptr, false);
+}
+// If we don't have sysroot or it didn't work then 
+// try original module spec.
+if (!error.Success()) {
+  resolved_spec = spec;
+  error = ModuleList::GetSharedModule(
+resolved_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr, 
+did_create_ptr, false);
+}
+if (error.Success() && module_sp)
+  module_sp->SetPlatformFileSpec(resolved_spec.GetFileSpec());
+return error;
+  };
+
   return GetRemoteSharedModule(module_spec, process, module_sp,
-   [&](const ModuleSpec ) {
- Status error = ModuleList::GetSharedModule(
- spec, module_sp, module_search_paths_ptr,
- old_module_sp_ptr, did_create_ptr, false);
- if (error.Success() && module_sp)
-   module_sp->SetPlatformFileSpec(
-   spec.GetFileSpec());
- return error;
-   },
-   did_create_ptr);
+   resolver, did_create_ptr);
 }
 
 bool Platform::GetModuleSpec(const FileSpec _file_spec,
Index: packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/TestLinuxCore.py
===
--- packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -6,6 +6,7 @@
 
 import shutil
 import struct
+import os
 
 import lldb
 from lldbsuite.test.decorators import *
@@ -203,6 +204,33 @@
 for regname, value in values.iteritems():
 self.expect("register read {}".format(regname), substrs=["{} = {}".format(regname, value)])
 
+@expectedFailureAll(bugnumber="llvm.org/pr37371", hostoslist=["windows"])
+@skipIf(triple='^mips')
+@skipIfLLVMTargetMissing("X86")
+def test_i386_sysroot(self):
+"""Test that lldb can find the exe for an i386 linux core file using the sysroot."""
+
+# Copy linux-i386.out to tmp_sysroot/home/labath/test/a.out (since it was compiled as
+# /home/labath/test/a.out)
+tmp_sysroot = "/tmp/lldb_i386_mock_sysroot"
+executable = tmp_sysroot + "/home/labath/test/a.out"
+lldbutil.mkdir_p(os.path.dirname(executable))
+shutil.copyfile("linux-i386.out", executable)
+
+# Set sysroot and load core
+self.runCmd("platform select remote-linux --sysroot '%s'" % tmp_sysroot)
+target = self.dbg.CreateTarget(None)
+self.assertTrue(target, VALID_TARGET)
+process = target.LoadCore("linux-i386.core")
+
+# Check that we found a.out from the sysroot
+self.check_all(process, self._i386_pid, self._i386_regions, "a.out")
+
+self.dbg.DeleteTarget(target)
+
+# Clean up tmp_sysroot
+shutil.rmtree(tmp_sysroot)
+
 def check_memory_regions(self, process, region_count):
 region_list = process.GetMemoryRegions()
 self.assertEqual(region_list.GetSize(), region_count)
@@ -299,15 +327,7 @@
 self.dbg.SetOutputFileHandle(None, False)
 self.dbg.SetErrorFileHandle(None, False)
 
-def do_test(self, filename, pid, region_count, thread_name):
-target = self.dbg.CreateTarget(filename + ".out")
-process = target.LoadCore(filename + ".core")
-self.assertTrue(process, PROCESS_IS_VALID)
-self.assertEqual(process.GetNumThreads(), 1)
-self.assertEqual(process.GetProcessID(), pid)
-
-self.check_state(process)
-
+def check_stack(self, process, pid, thread_name):
 thread = 

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi added a comment.

In https://reviews.llvm.org/D49685#1201351, @magardne wrote:

> @labath Eugene asked me to help add a unit test for this. I have the updated 
> diff, but I can't seem to attach it to this code review -- it must be because 
> I'm not the original author? I'll attach the diff to this comment and maybe 
> you can try to update the review. Otherwise we can wait until Eugene gets 
> back on Monday.
>
> The diff makes the following changes:
>
> - Add an executable: 
> test/testcases/functionalities/postmortem/elf-core/mock_sysroot/home/labath/test/a.out
> - This is a copy of linux-i386.out (which is why it's in 
> mock_sysroot/home/labath...). Let me know if this is ok, or if I should try 
> to create a new binary/core
> - Implement the test you suggested: set platform and sysroot, open the core, 
> check threads/stacks are valid
>
>   I verified that the test fails without the changes to Platform.cpp and 
> passes after the changes.
>
>   F6945416: sysroot_cr.diff 


Mark, thanks a lot for doing this. I have one request though: let's not add new 
binaries to the repo. Instead, the test should create a temporary directory 
somewhere (in /tmp, I guess) and copy necessary files there. After the test is 
done, wipe it out.


https://reviews.llvm.org/D49685



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi added a comment.

In https://reviews.llvm.org/D49685#1179837, @labath wrote:

> In https://reviews.llvm.org/D49685#1178730, @EugeneBi wrote:
>
> > I looked at the tests - is it all in Python? Not sure I have time to learn 
> > a new language... Is there anything in C++?
>
>
> We have unit tests in c++, but it's going to be quite hard to tickle this 
> code path from there.
>
> FWIW, I don't think you really need to *know* python to write a test like 
> this. You should be able to fudge it by cargo-culting some code from existing 
> tests and some basic python examples. I expect the test should be something 
> like:
>
>   # copy core file an .exe into an appropriate directory tree
>   self.runCmd("platform select remote-linux --sysroot '%s'" % sysroot)
>   target = self.dbg.CreateTarget(None)
>   process = target.LoadCode(core)
>   self.assertEquals(1, target.GetNumModules())
>   self.assertEquals(exe, target.GetModuleAtIndex(0).GetFileSpec())
>


I am going on vacation till the end of August and I still have something to do 
at work. Most likely I will not have a chance to look at it until I am back.


https://reviews.llvm.org/D49685



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi added a comment.

In https://reviews.llvm.org/D49685#1178553, @labath wrote:

> In https://reviews.llvm.org/D49685#1178528, @EugeneBi wrote:
>
> > Hmm... I never thought I can do that :)
> >  Anyway, with the change as it is now, LLDB would try to load executable in 
> > the sysroot, fail that, then open it without the sysroot.
>
>
> Does that mean it is possible to exercise this code path (by placing the 
> executable in the sysroot for lldb to find it)?


I looked at the tests - is it all in Python? Not sure I have time to learn a 
new language... Is there anything in C++?


https://reviews.llvm.org/D49685



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi added a comment.

In https://reviews.llvm.org/D49685#1178543, @lemo wrote:

> > I never *ran LLDB tests*, not sure where they are and what they are.
>
> I hope you're planning to look into this before submitting the change :)


Good idea :)

Scanning dependencies of target check-lldb
[100%] Built target check-lldb


https://reviews.llvm.org/D49685



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi added a comment.

Hmm... I never thought I can do that :)
Anyway, with the change as it is now, LLDB would try to load executable in the 
sysroot, fail that, then open it without the sysroot.


https://reviews.llvm.org/D49685



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi added a comment.

In https://reviews.llvm.org/D49685#1178125, @labath wrote:

> In https://reviews.llvm.org/D49685#1177046, @EugeneBi wrote:
>
> > It is specific to shared libraries. Opening the executable and core dump 
> > follows different code path.
>
>
> Which path is that? Is the path different even when you don't specify an 
> executable when opening the core file (`target create --core /my/file.core`). 
> I believe that in this case we should end up here 
> https://github.com/llvm-mirror/lldb/blob/7c07b8a9314eef118f95b57b49fa099be0808eac/source/Plugins/Process/elf-core/ProcessElfCore.cpp#L255,
>  which should eventually call `Platform::GetSharedModule`. Is that not the 
> case?


I did not try not to specify executable - what's the point in doing that? LLDB 
would fail to find symbols.

Here is the stack I captured when I debugged the problem. I believe that 
DynamicLoaderPOSIXDYLD::LoadAllCurrentModules did not try to load the 
executable.

(gdb) bt
#0  lldb_private::ModuleList::GetSharedModule (module_spec=..., 
module_sp=std::shared_ptr (empty) 0x0,

  module_search_paths_ptr=0x83ad60, old_module_sp_ptr=0x7fffbb50, 
did_create_ptr=0x7fffbb07,
  always_create=false) at 
/home/eugene/llvm/tools/lldb/source/Core/ModuleList.cpp:710

#1  0x7fffedc2d130 in lldb_private::Platformoperator()(const lldb_private::ModuleSpec &) const 
(__closure=0x8581b0, spec=...)

  at /home/eugene/llvm/tools/lldb/source/Target/Platform.cpp:234

#2  0x7fffedc34ff2 in std::_Function_handler >::_M_invoke(const std::_Any_data &, const 
lldb_private::ModuleSpec &) (__functor=..., __args#0=...) at 
/usr/include/c++/5/functional:1857
#3  0x7fffedc37978 in std::function::operator()(lldb_private::ModuleSpec const&) 
const (this=0x7fffba80, __args#0=...) at /usr/include/c++/5/functional:2267
#4  0x7fffedc3375a in 
lldb_private::Platform::GetRemoteSharedModule(lldb_private::ModuleSpec const&, 
lldb_private::Process*, std::shared_ptr&, 
std::function const&, 
bool*) (this=0x839330, module_spec=..., process=0x84d310, 
module_sp=std::shared_ptr (empty) 0x0,

  module_resolver=..., did_create_ptr=0x7fffbb07)
  at /home/eugene/llvm/tools/lldb/source/Target/Platform.cpp:1628

#5  0x7fffedc2d2cd in lldb_private::Platform::GetSharedModule 
(this=0x839330, module_spec=...,

  process=0x84d310, module_sp=std::shared_ptr (empty) 0x0, 
module_search_paths_ptr=0x83ad60,
  old_module_sp_ptr=0x7fffbb50, did_create_ptr=0x7fffbb07)
  at /home/eugene/llvm/tools/lldb/source/Target/Platform.cpp:240

#6  0x7fffedc9957c in lldb_private::Target::GetSharedModule (this=0x846960, 
module_spec=..., error_ptr=0x0)

  at /home/eugene/llvm/tools/lldb/source/Target/Target.cpp:1952

#7  0x7fffef8e0d11 in lldb_private::DynamicLoader::LoadModuleAtAddress 
(this=0x9a0a70, file=...,

  link_map_addr=139700267943784, base_addr=139700263510016, 
base_addr_is_offset=true)
  at /home/eugene/llvm/tools/lldb/source/Core/DynamicLoader.cpp:171

#8  0x7fffedd8fb55 in DynamicLoaderPOSIXDYLD::LoadAllCurrentModules 
(this=0x9a0a70)

  at 
/home/eugene/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:537

#9  0x7fffedd8de52 in DynamicLoaderPOSIXDYLD::DidAttach (this=0x9a0a70)

  at 
/home/eugene/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:171

#10 0x7fffedc476d9 in lldb_private::Process::LoadCore (this=0x84d310)

  at /home/eugene/llvm/tools/lldb/source/Target/Process.cpp:2853


https://reviews.llvm.org/D49685



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi added a comment.

In https://reviews.llvm.org/D49685#1176399, @labath wrote:

> In https://reviews.llvm.org/D49685#1175413, @EugeneBi wrote:
>
> > In https://reviews.llvm.org/D49685#1174770, @labath wrote:
> >
> > > Could you also add a test for this?
> >
> >
> > I never ran LLDB tests, not sure where they are and what they are.
> >  Also, how would you test that? I know now my open core dump works, but I 
> > cannot share it.
>
>
> Good question. Is this functionality specific to shared libraries, or could 
> it be used to locate the main exe too?
>
> If yes, then it should be sufficient to take one of the existing core 
> files we have, copy the exe into a path matching what the core file expects, 
> set the sysroot, and make sure the exe gets found when we open up the core 
> file. You can look at the existing tests in 
> `test/testcases/functionalities/postmortem/elf-core` for examples.
>
> If not, then it might get trickier because we'll need new core files, and 
> it's hard to generate small ones with shared libraries.


It is specific to shared libraries. Opening the executable and core dump 
follows different code path.


https://reviews.llvm.org/D49685



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi added a comment.

In https://reviews.llvm.org/D49685#1174770, @labath wrote:

> Could you also add a test for this?


I never ran LLDB tests, not sure where they are and what they are.
Also, how would you test that? I know now my open core dump works, but I cannot 
share it.


https://reviews.llvm.org/D49685



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi added inline comments.



Comment at: source/Target/Platform.cpp:252
+if (error.Success() && module_sp)
+  module_sp->SetPlatformFileSpec(spec.GetFileSpec());
+return error;

EugeneBi wrote:
> A bug here. must be resolved_spec if it succeeds.
Now I have my doubts: what "platform file spec" really means? I mean, which code
is correct: current or previous one?


https://reviews.llvm.org/D49685



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi updated this revision to Diff 157146.
EugeneBi added a comment.

Fix a bug with resolved_spec path.


https://reviews.llvm.org/D49685

Files:
  source/Target/Platform.cpp


Index: source/Target/Platform.cpp
===
--- source/Target/Platform.cpp
+++ source/Target/Platform.cpp
@@ -228,17 +228,36 @@
 module_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr,
 did_create_ptr, false);
 
+  // Module resolver lambda.
+  auto resolver = [&](const ModuleSpec ) {
+Status error(eErrorTypeGeneric);
+ModuleSpec resolved_spec;
+// Check if we have sysroot set.
+if (m_sdk_sysroot) {
+  // Prepend sysroot to module spec.
+  resolved_spec = spec;
+  resolved_spec.GetFileSpec().PrependPathComponent(
+m_sdk_sysroot.GetStringRef());
+  // Try to get shared module with resolved spec.
+  error = ModuleList::GetSharedModule( 
+resolved_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr, 
+did_create_ptr, false);
+}
+// If we don't have sysroot or it didn't work then 
+// try original module spec.
+if (!error.Success()) {
+  resolved_spec = spec;
+  error = ModuleList::GetSharedModule(
+resolved_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr, 
+did_create_ptr, false);
+}
+if (error.Success() && module_sp)
+  module_sp->SetPlatformFileSpec(resolved_spec.GetFileSpec());
+return error;
+  };
+
   return GetRemoteSharedModule(module_spec, process, module_sp,
-   [&](const ModuleSpec ) {
- Status error = ModuleList::GetSharedModule(
- spec, module_sp, module_search_paths_ptr,
- old_module_sp_ptr, did_create_ptr, false);
- if (error.Success() && module_sp)
-   module_sp->SetPlatformFileSpec(
-   spec.GetFileSpec());
- return error;
-   },
-   did_create_ptr);
+   resolver, did_create_ptr);
 }
 
 bool Platform::GetModuleSpec(const FileSpec _file_spec,


Index: source/Target/Platform.cpp
===
--- source/Target/Platform.cpp
+++ source/Target/Platform.cpp
@@ -228,17 +228,36 @@
 module_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr,
 did_create_ptr, false);
 
+  // Module resolver lambda.
+  auto resolver = [&](const ModuleSpec ) {
+Status error(eErrorTypeGeneric);
+ModuleSpec resolved_spec;
+// Check if we have sysroot set.
+if (m_sdk_sysroot) {
+  // Prepend sysroot to module spec.
+  resolved_spec = spec;
+  resolved_spec.GetFileSpec().PrependPathComponent(
+m_sdk_sysroot.GetStringRef());
+  // Try to get shared module with resolved spec.
+  error = ModuleList::GetSharedModule( 
+resolved_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr, 
+did_create_ptr, false);
+}
+// If we don't have sysroot or it didn't work then 
+// try original module spec.
+if (!error.Success()) {
+  resolved_spec = spec;
+  error = ModuleList::GetSharedModule(
+resolved_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr, 
+did_create_ptr, false);
+}
+if (error.Success() && module_sp)
+  module_sp->SetPlatformFileSpec(resolved_spec.GetFileSpec());
+return error;
+  };
+
   return GetRemoteSharedModule(module_spec, process, module_sp,
-   [&](const ModuleSpec ) {
- Status error = ModuleList::GetSharedModule(
- spec, module_sp, module_search_paths_ptr,
- old_module_sp_ptr, did_create_ptr, false);
- if (error.Success() && module_sp)
-   module_sp->SetPlatformFileSpec(
-   spec.GetFileSpec());
- return error;
-   },
-   did_create_ptr);
+   resolver, did_create_ptr);
 }
 
 bool Platform::GetModuleSpec(const FileSpec _file_spec,
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi added inline comments.



Comment at: source/Target/Platform.cpp:252
+if (error.Success() && module_sp)
+  module_sp->SetPlatformFileSpec(spec.GetFileSpec());
+return error;

A bug here. must be resolved_spec if it succeeds.


https://reviews.llvm.org/D49685



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi updated this revision to Diff 157135.
EugeneBi marked 6 inline comments as done.
EugeneBi added a comment.

Code review followup:

- Restricted change to Platform.cpp
- Restricted change only to remote platforms.


https://reviews.llvm.org/D49685

Files:
  source/Target/Platform.cpp


Index: source/Target/Platform.cpp
===
--- source/Target/Platform.cpp
+++ source/Target/Platform.cpp
@@ -228,17 +228,33 @@
 module_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr,
 did_create_ptr, false);
 
+  // Module resolver lambda.
+  auto resolver = [&](const ModuleSpec ) {
+Status error(eErrorTypeGeneric);
+// Check if we have sysroot set.
+if (m_sdk_sysroot) {
+  // Prepend sysroot to module spec.
+  auto resolved_spec(spec);
+  resolved_spec.GetFileSpec().PrependPathComponent(
+m_sdk_sysroot.GetStringRef());
+  // Try to get shared module with resolved spec.
+  error = ModuleList::GetSharedModule( 
+resolved_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr, 
+did_create_ptr, false);
+}
+// If we don't have sysroot or it didn't work then 
+// try original module spec.
+if (!error.Success())
+  error = ModuleList::GetSharedModule(
+spec, module_sp, module_search_paths_ptr, old_module_sp_ptr, 
+did_create_ptr, false);
+if (error.Success() && module_sp)
+  module_sp->SetPlatformFileSpec(spec.GetFileSpec());
+return error;
+  };
+
   return GetRemoteSharedModule(module_spec, process, module_sp,
-   [&](const ModuleSpec ) {
- Status error = ModuleList::GetSharedModule(
- spec, module_sp, module_search_paths_ptr,
- old_module_sp_ptr, did_create_ptr, false);
- if (error.Success() && module_sp)
-   module_sp->SetPlatformFileSpec(
-   spec.GetFileSpec());
- return error;
-   },
-   did_create_ptr);
+   resolver, did_create_ptr);
 }
 
 bool Platform::GetModuleSpec(const FileSpec _file_spec,


Index: source/Target/Platform.cpp
===
--- source/Target/Platform.cpp
+++ source/Target/Platform.cpp
@@ -228,17 +228,33 @@
 module_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr,
 did_create_ptr, false);
 
+  // Module resolver lambda.
+  auto resolver = [&](const ModuleSpec ) {
+Status error(eErrorTypeGeneric);
+// Check if we have sysroot set.
+if (m_sdk_sysroot) {
+  // Prepend sysroot to module spec.
+  auto resolved_spec(spec);
+  resolved_spec.GetFileSpec().PrependPathComponent(
+m_sdk_sysroot.GetStringRef());
+  // Try to get shared module with resolved spec.
+  error = ModuleList::GetSharedModule( 
+resolved_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr, 
+did_create_ptr, false);
+}
+// If we don't have sysroot or it didn't work then 
+// try original module spec.
+if (!error.Success())
+  error = ModuleList::GetSharedModule(
+spec, module_sp, module_search_paths_ptr, old_module_sp_ptr, 
+did_create_ptr, false);
+if (error.Success() && module_sp)
+  module_sp->SetPlatformFileSpec(spec.GetFileSpec());
+return error;
+  };
+
   return GetRemoteSharedModule(module_spec, process, module_sp,
-   [&](const ModuleSpec ) {
- Status error = ModuleList::GetSharedModule(
- spec, module_sp, module_search_paths_ptr,
- old_module_sp_ptr, did_create_ptr, false);
- if (error.Success() && module_sp)
-   module_sp->SetPlatformFileSpec(
-   spec.GetFileSpec());
- return error;
-   },
-   did_create_ptr);
+   resolver, did_create_ptr);
 }
 
 bool Platform::GetModuleSpec(const FileSpec _file_spec,
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi added a comment.

> You would just move:
> 
>   auto resolved_module_spec(module_spec);
> if (sysroot != nullptr)
>   resolved_module_spec.GetFileSpec().PrependPathComponent(sysroot);
> 
> 
> into the code in Platform.cpp and do it all there.

Ah, I see. Will do, thanks.


https://reviews.llvm.org/D49685



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi added a comment.

I need to convert char* to StringRef yet.


https://reviews.llvm.org/D49685



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi updated this revision to Diff 157088.
EugeneBi added a comment.

Rebased to recent master.
Included the whole file in diff.


https://reviews.llvm.org/D49685

Files:
  include/lldb/Core/ModuleList.h
  source/Core/ModuleList.cpp
  source/Target/Platform.cpp


Index: source/Target/Platform.cpp
===
--- source/Target/Platform.cpp
+++ source/Target/Platform.cpp
@@ -226,13 +226,14 @@
   if (IsHost())
 return ModuleList::GetSharedModule(
 module_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr,
-did_create_ptr, false);
+did_create_ptr, false, m_sdk_sysroot.AsCString());
 
   return GetRemoteSharedModule(module_spec, process, module_sp,
[&](const ModuleSpec ) {
  Status error = ModuleList::GetSharedModule(
  spec, module_sp, module_search_paths_ptr,
- old_module_sp_ptr, did_create_ptr, false);
+ old_module_sp_ptr, did_create_ptr, false,
+ m_sdk_sysroot.AsCString());
  if (error.Success() && module_sp)
module_sp->SetPlatformFileSpec(
spec.GetFileSpec());
Index: source/Core/ModuleList.cpp
===
--- source/Core/ModuleList.cpp
+++ source/Core/ModuleList.cpp
@@ -770,7 +770,11 @@
ModuleSP _sp,
const FileSpecList *module_search_paths_ptr,
ModuleSP *old_module_sp_ptr,
-   bool *did_create_ptr, bool always_create) {
+   bool *did_create_ptr, bool always_create,
+   const char* sysroot) {
+  // Make sure no one else can try and get or create a module while this
+  // function is actively working on it by doing an extra lock on the
+  // global mutex list.
   ModuleList _module_list = GetSharedModuleList();
   std::lock_guard guard(
   shared_module_list.m_modules_mutex);
@@ -789,9 +793,6 @@
   const FileSpec _file_spec = module_spec.GetFileSpec();
   const ArchSpec  = module_spec.GetArchitecture();
 
-  // Make sure no one else can try and get or create a module while this
-  // function is actively working on it by doing an extra lock on the global
-  // mutex list.
   if (!always_create) {
 ModuleList matching_module_list;
 const size_t num_matching_modules =
@@ -825,7 +826,11 @@
   if (module_sp)
 return error;
 
-  module_sp.reset(new Module(module_spec));
+  auto resolved_module_spec(module_spec);
+  if (sysroot != nullptr)
+resolved_module_spec.GetFileSpec().PrependPathComponent(sysroot);
+
+  module_sp.reset(new Module(resolved_module_spec));
   // Make sure there are a module and an object file since we can specify a
   // valid file path with an architecture that might not be in that file. By
   // getting the object file we can guarantee that the architecture matches
Index: include/lldb/Core/ModuleList.h
===
--- include/lldb/Core/ModuleList.h
+++ include/lldb/Core/ModuleList.h
@@ -537,7 +537,8 @@
 const FileSpecList *module_search_paths_ptr,
 lldb::ModuleSP *old_module_sp_ptr,
 bool *did_create_ptr,
-bool always_create = false);
+bool always_create = false,
+const char* sysroot = nullptr);
 
   static bool RemoveSharedModule(lldb::ModuleSP _sp);
 


Index: source/Target/Platform.cpp
===
--- source/Target/Platform.cpp
+++ source/Target/Platform.cpp
@@ -226,13 +226,14 @@
   if (IsHost())
 return ModuleList::GetSharedModule(
 module_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr,
-did_create_ptr, false);
+did_create_ptr, false, m_sdk_sysroot.AsCString());
 
   return GetRemoteSharedModule(module_spec, process, module_sp,
[&](const ModuleSpec ) {
  Status error = ModuleList::GetSharedModule(
  spec, module_sp, module_search_paths_ptr,
- old_module_sp_ptr, did_create_ptr, false);
+ old_module_sp_ptr, did_create_ptr, false,
+ m_sdk_sysroot.AsCString());
  if (error.Success() && module_sp)
module_sp->SetPlatformFileSpec(
spec.GetFileSpec());
Index: 

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi added a comment.

In https://reviews.llvm.org/D49685#1173640, @clayborg wrote:

> We should never be loading the wrong shared libraries. The module spec we 
> fill out must contain the UUID of the file are looking for. If it doesn't we 
> have no chance of every really loading the right stuff.


Well, that's what it does on my machine. So prepending sysroot *after* trying 
to load module without it will cause problems to me. Also, I assume that if you 
specified sysroot for a platform, we should not try to load solibs without it - 
these paths are just not a part of the platform.

> I think doing this in the module list is not the right place. Why? Some 
> platforms might have multiple sysroot to check. iOS for instance has a 
> directory for each device that Xcode has connected to which can be checked. I 
> am fine with adding this ability to>  lldb_private::Platform, but I would 
> just do it in there. Try GetRemoteSharedModule with the spec, if it fails, 
> try again after modifying the spec to prepend the sysroot path. Possible even 
> just check the sysroot path + path first if m_sdk_sysroot is filled in. I 
> don't see the need to change ModuleList itself.

I do not see how this prepend sysroot could be done in the platform... 
Essentially, my fix is doing what you suggest: the platform supplies sysroot to 
the module list, and two different platforms (for two XCode devices, etc.) 
would supply different sysroots. What do I miss?


Repository:
  rL LLVM

https://reviews.llvm.org/D49685



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D29095: Open ELF core dumps with more than 64K sections

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi added a comment.

So, what's now? Can somebody commit it, please? 
I do not see any option to do it myself.


https://reviews.llvm.org/D29095



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D29095: Open ELF core dumps with more than 64K sections

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi updated this revision to Diff 86148.
EugeneBi added a comment.

Used named constants for SHN_UNDEF and SHN_XINDEX sentinels. 
Unfortunately ELF.h lacks definition of PN_XNUM, so left it as a comment.


https://reviews.llvm.org/D29095

Files:
  source/Plugins/ObjectFile/ELF/ELFHeader.cpp
  source/Plugins/ObjectFile/ELF/ELFHeader.h
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  source/Plugins/Process/elf-core/ProcessElfCore.cpp
  unittests/CMakeLists.txt
  unittests/ObjectFile/CMakeLists.txt
  unittests/ObjectFile/ELF/CMakeLists.txt
  unittests/ObjectFile/ELF/TestELFHeader.cpp

Index: unittests/ObjectFile/ELF/TestELFHeader.cpp
===
--- /dev/null
+++ unittests/ObjectFile/ELF/TestELFHeader.cpp
@@ -0,0 +1,62 @@
+//===-- TestELFHeader.cpp ---*- C++ -*-===//
+//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Plugins/ObjectFile/ELF/ELFHeader.h"
+#include "lldb/Core/DataExtractor.h"
+#include "gtest/gtest.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+
+TEST(ELFHeader, ParseHeaderExtension) {
+  uint8_t data[] = {
+  // e_ident
+  0x7f, 0x45, 0x4c, 0x46, 0x02, 0x01, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00,
+  0x00, 0x00, 0x00, 0x00,
+
+  // e_type, e_machine, e_version, e_entry
+  0x03, 0x00, 0x3e, 0x00, 0x01, 0x00, 0x00, 0x00, 0x90, 0x48, 0x40, 0x00,
+  0x00, 0x00, 0x00, 0x00,
+
+  // e_phoff, e_shoff
+  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x40, 0x00, 0x00, 0x00,
+  0x00, 0x00, 0x00, 0x00,
+
+  // e_flags, e_ehsize, e_phentsize, e_phnum, e_shentsize, e_shnum,
+  // e_shstrndx
+  0x00, 0x00, 0x00, 0x00, 0x40, 0x00, 0x38, 0x00, 0xff, 0xff, 0x40, 0x00,
+  0x00, 0x00, 0xff, 0xff,
+
+  // sh_name, sh_type, sh_flags
+  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+  0x00, 0x00, 0x00, 0x00,
+
+  // sh_addr, sh_offset
+  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+  0x00, 0x00, 0x00, 0x00,
+
+  // sh_size, sh_link, sh_info
+  0x23, 0x45, 0x67, 0x00, 0x00, 0x00, 0x00, 0x00, 0x34, 0x56, 0x78, 0x00,
+  0x12, 0x34, 0x56, 0x00,
+
+  // sh_addralign, sh_entsize
+  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+  0x00, 0x00, 0x00, 0x00,
+  };
+
+  DataExtractor extractor(data, sizeof data, eByteOrderLittle, 8);
+  elf::ELFHeader header;
+  offset_t offset = 0;
+  ASSERT_TRUE(header.Parse(extractor, ));
+  EXPECT_EQ(0x563412u, header.e_phnum);
+  EXPECT_EQ(0x785634u, header.e_shstrndx);
+  EXPECT_EQ(0x674523u, header.e_shnum);
+}
Index: unittests/ObjectFile/ELF/CMakeLists.txt
===
--- /dev/null
+++ unittests/ObjectFile/ELF/CMakeLists.txt
@@ -0,0 +1,3 @@
+add_lldb_unittest(ObjectFileELFTests
+  TestELFHeader.cpp
+  )
Index: unittests/ObjectFile/CMakeLists.txt
===
--- /dev/null
+++ unittests/ObjectFile/CMakeLists.txt
@@ -0,0 +1 @@
+add_subdirectory(ELF)
Index: unittests/CMakeLists.txt
===
--- unittests/CMakeLists.txt
+++ unittests/CMakeLists.txt
@@ -53,6 +53,7 @@
 add_subdirectory(Host)
 add_subdirectory(Interpreter)
 add_subdirectory(Language)
+add_subdirectory(ObjectFile)
 add_subdirectory(Platform)
 add_subdirectory(Process)
 add_subdirectory(ScriptInterpreter)
Index: source/Plugins/Process/elf-core/ProcessElfCore.cpp
===
--- source/Plugins/Process/elf-core/ProcessElfCore.cpp
+++ source/Plugins/Process/elf-core/ProcessElfCore.cpp
@@ -56,6 +56,8 @@
   lldb::ProcessSP process_sp;
   if (crash_file) {
 // Read enough data for a ELF32 header or ELF64 header
+// Note: Here we care about e_type field only, so it is safe
+// to ignore possible presence of the header extension.
 const size_t header_size = sizeof(llvm::ELF::Elf64_Ehdr);
 
 lldb::DataBufferSP data_sp(crash_file->ReadFileContents(0, header_size));
Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -610,7 +610,8 @@
 DataExtractor data;
 data.SetData(data_sp);
 elf::ELFHeader header;
-if (header.Parse(data, _offset)) {
+lldb::offset_t header_offset = data_offset;
+if (header.Parse(data, _offset)) {
   if (data_sp) {
 ModuleSpec spec(file);
 
@@ -645,10 +646,24 @@
   __FUNCTION__, file.GetPath().c_str());
   }
 
+  

[Lldb-commits] [PATCH] D29095: Open ELF core dumps with more than 64K sections

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi added inline comments.



Comment at: source/Plugins/ObjectFile/ELF/ELFHeader.cpp:108
+  if (ok) {
+if (e_phnum_hdr == 0x)
+  e_phnum = section_zero.sh_info;

EugeneBi wrote:
> EugeneBi wrote:
> > davidb wrote:
> > > Would this make more sense to compare against a named constant 
> > > ELF::PN_XNUM?
> > Would be nice, but - where is it defined? I tried ELF::PN_XNUM, 
> > elf::PN_XNUM, PN_XNUM - compiler barks anyway.
> > 
> OK, I see the other two are defined in Support/ELF.h, I'll put PN_XNUM there 
> then.
Oh, this ELF.h is a part of a different git repo - it lives in llvm. I am not 
sure what to do about it.


https://reviews.llvm.org/D29095



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D29095: Open ELF core dumps with more than 64K sections

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi added inline comments.



Comment at: source/Plugins/ObjectFile/ELF/ELFHeader.cpp:108
+  if (ok) {
+if (e_phnum_hdr == 0x)
+  e_phnum = section_zero.sh_info;

EugeneBi wrote:
> davidb wrote:
> > Would this make more sense to compare against a named constant ELF::PN_XNUM?
> Would be nice, but - where is it defined? I tried ELF::PN_XNUM, elf::PN_XNUM, 
> PN_XNUM - compiler barks anyway.
> 
OK, I see the other two are defined in Support/ELF.h, I'll put PN_XNUM there 
then.


https://reviews.llvm.org/D29095



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D29095: Open ELF core dumps with more than 64K sections

[Eugene Birukov via Phabricator via lldb-commits]

EugeneBi added inline comments.



Comment at: source/Plugins/ObjectFile/ELF/ELFHeader.cpp:108
+  if (ok) {
+if (e_phnum_hdr == 0x)
+  e_phnum = section_zero.sh_info;

davidb wrote:
> Would this make more sense to compare against a named constant ELF::PN_XNUM?
Would be nice, but - where is it defined? I tried ELF::PN_XNUM, elf::PN_XNUM, 
PN_XNUM - compiler barks anyway.



https://reviews.llvm.org/D29095



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits