[ExecutionContext] Return the target/process byte order.

[JDevlieghere]

Summary

Currently ExecutionContext::GetByteOrder() always returns the host byte order. This seems like a simple mistake: the return keyword appears to have been omitted by accident. This patch fixes that and adds a unit test.

Fixes https://llvm.org/PR37950

[ramana-nvr]

Created the patch with more context.

[davide]

Can you write a unittest for this? Thanks.

[teemperor]

Do we have a test case for different target byte orders that could be used as a test template here? Otherwise I would just merge this in as the previous behavior of this function was clearly wrong.

[ramana-nvr]

Sorry, I don't have a test case for this and I am yet to understand lldb test suite to create one.

[teemperor]

My question about a good test case template was more directed at everyone :)

[clayborg]

Should be easy to write unit test for ExecutionContext.cpp. See FileSpecTest.cpp for how to implement a unit test for a .cpp file.

[JDevlieghere]

Add unit test

[labath]

Thanks for setting this up. The infrastructure is a bit overkill for a simple test like this, but hopefully this can be useful for other tests too. I have a bunch of comments, but hopefully none of them are major, so if you agree with all of them, just fire away..

lldb/unittests/Target/ExecutionContextTest.cpp

40–43 ↗

(On Diff #213797)

call terminate in reverse order?

77 ↗

(On Diff #213797)

I don't understand this.. Why do you fetch the ArchSpec via Target::GetDefaultArchitecture() and then immediately overwrite it on the next line? Can this line just be deleted?

78 ↗

(On Diff #213797)

It looks weird to have an apple triple with a linux platform. I am assuming you use the linux platform because it can be compiled everywhere, but in that case, maybe you could use a linux triple too? Maybe also use a big-endian triple in one of the tests to catch regressions regardless of the endianness of the host?

85 ↗

(On Diff #213797)

change EXPECT_TRUE to ASSERT_TRUE. There's no point in continuing the test if this is false, as it will just crash anyway...

124 ↗

(On Diff #213797)

It doesn't look like the presence of a process will change what ExecutionContext::GetByteOrder returns, as it checks the target first. OTOH, I don't know why would the byte order of a process be ever different from the byte order of its target...

[JDevlieghere]

Code review feedback

[JDevlieghere]

lldb/unittests/Target/ExecutionContextTest.cpp

77 ↗

(On Diff #213797)

Yep, I uploaded an old version of the patch. This and the triple should've already been fixed.

124 ↗

(On Diff #213797)

Yeah, I think the logic in the implementation is redundant, this is more of a test to prevent regressions in case the implementation would change in the future.

lldb/trunk/source/Target/ExecutionContext.cpp

if (m_target_sp && m_target_sp->GetArchitecture().IsValid())
     return m_target_sp->GetArchitecture().GetAddressByteSize();
   if (m_process_sp)
     return m_process_sp->GetAddressByteSize();
   return sizeof(void *);
 }
 
 lldb::ByteOrder ExecutionContext::GetByteOrder() const {
   if (m_target_sp && m_target_sp->GetArchitecture().IsValid())
-    m_target_sp->GetArchitecture().GetByteOrder();
+    return m_target_sp->GetArchitecture().GetByteOrder();
   if (m_process_sp)
-    m_process_sp->GetByteOrder();
+    return m_process_sp->GetByteOrder();
   return endian::InlHostByteOrder();
 }
 
 RegisterContext *ExecutionContext::GetRegisterContext() const {
   if (m_frame_sp)
     return m_frame_sp->GetRegisterContext().get();
   else if (m_thread_sp)
     return m_thread_sp->GetRegisterContext().get();

lldb/trunk/unittests/Target/CMakeLists.txt

 add_lldb_unittest(TargetTests
+  ExecutionContextTest.cpp
   MemoryRegionInfoTest.cpp
   ModuleCacheTest.cpp
   PathMappingListTest.cpp
   ProcessInstanceInfoTest.cpp
 
   LINK_LIBS
       lldbCore
       lldbHost
-      lldbSymbol
-      lldbUtility
       lldbPluginObjectFileELF
+      lldbPluginPlatformLinux
       lldbPluginSymbolFileSymtab
+      lldbSymbol
+      lldbUtility
       lldbUtilityHelpers
     LINK_COMPONENTS
       Support
   )
 
 add_unittest_inputs(TargetTests TestModule.so)

lldb/trunk/unittests/Target/ExecutionContextTest.cpp

+//===-- ExecutionContextTest.cpp --------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Target/ExecutionContext.h"
+#include "Plugins/Platform/Linux/PlatformLinux.h"
+#include "lldb/Core/Debugger.h"
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Target/Platform.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Target/Target.h"
+#include "lldb/Utility/ArchSpec.h"
+#include "lldb/Utility/Endian.h"
+#include "lldb/Utility/Reproducer.h"
+#include "lldb/lldb-enumerations.h"
+#include "lldb/lldb-forward.h"
+#include "lldb/lldb-private-enumerations.h"
+#include "lldb/lldb-private.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace lldb_private::repro;
+using namespace lldb;
+
+namespace {
+class ExecutionContextTest : public ::testing::Test {
+public:
+  void SetUp() override {
+    llvm::cantFail(Reproducer::Initialize(ReproducerMode::Off, llvm::None));
+    FileSystem::Initialize();
+    HostInfo::Initialize();
+    platform_linux::PlatformLinux::Initialize();
+  }
+  void TearDown() override {
+    platform_linux::PlatformLinux::Terminate();
+    HostInfo::Terminate();
+    FileSystem::Terminate();
+    Reproducer::Terminate();
+  }
+};
+
+class DummyProcess : public Process {
+public:
+  using Process::Process;
+
+  virtual bool CanDebug(lldb::TargetSP target, bool plugin_specified_by_name) {
+    return true;
+  }
+  virtual Status DoDestroy() { return {}; }
+  virtual void RefreshStateAfterStop() {}
+  virtual size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size,
+                              Status &error) {
+    return 0;
+  }
+  virtual bool UpdateThreadList(ThreadList &old_thread_list,
+                                ThreadList &new_thread_list) {
+    return false;
+  }
+  virtual ConstString GetPluginName() { return ConstString("Dummy"); }
+  virtual uint32_t GetPluginVersion() { return 0; }
+};
+} // namespace
+
+TEST_F(ExecutionContextTest, GetByteOrder) {
+  ExecutionContext exe_ctx(nullptr, nullptr, nullptr);
+  EXPECT_EQ(endian::InlHostByteOrder(), exe_ctx.GetByteOrder());
+}
+
+TEST_F(ExecutionContextTest, GetByteOrderTarget) {
+  ArchSpec arch("powerpc64-pc-linux");
+
+  Platform::SetHostPlatform(
+      platform_linux::PlatformLinux::CreateInstance(true, &arch));
+
+  DebuggerSP debugger_sp = Debugger::CreateInstance();
+  ASSERT_TRUE(debugger_sp);
+
+  TargetSP target_sp;
+  PlatformSP platform_sp;
+  Status error = debugger_sp->GetTargetList().CreateTarget(
+      *debugger_sp, "", arch, eLoadDependentsNo, platform_sp, target_sp);
+  ASSERT_TRUE(target_sp);
+  ASSERT_TRUE(target_sp->GetArchitecture().IsValid());
+  ASSERT_TRUE(platform_sp);
+
+  ExecutionContext target_ctx(target_sp, false);
+  EXPECT_EQ(target_sp->GetArchitecture().GetByteOrder(),
+            target_ctx.GetByteOrder());
+}
+
+TEST_F(ExecutionContextTest, GetByteOrderProcess) {
+  ArchSpec arch("powerpc64-pc-linux");
+
+  Platform::SetHostPlatform(
+      platform_linux::PlatformLinux::CreateInstance(true, &arch));
+
+  DebuggerSP debugger_sp = Debugger::CreateInstance();
+  ASSERT_TRUE(debugger_sp);
+
+  TargetSP target_sp;
+  PlatformSP platform_sp;
+  Status error = debugger_sp->GetTargetList().CreateTarget(
+      *debugger_sp, "", arch, eLoadDependentsNo, platform_sp, target_sp);
+  ASSERT_TRUE(target_sp);
+  ASSERT_TRUE(target_sp->GetArchitecture().IsValid());
+  ASSERT_TRUE(platform_sp);
+
+  ListenerSP listener_sp(Listener::MakeListener("dummy"));
+  ProcessSP process_sp = std::make_shared<DummyProcess>(target_sp, listener_sp);
+  ASSERT_TRUE(process_sp);
+
+  ExecutionContext process_ctx(process_sp);
+  EXPECT_EQ(process_sp->GetByteOrder(), process_ctx.GetByteOrder());
+}