[Lldb-commits] [PATCH] D80807: [lldb/Utility] Fix DecodeUUIDBytesFromString not to access past the input buffer

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

In D80807#2063196 , @friss wrote:

> I would have committed this right away if it weren't for the slight change in 
> behavior I wanted to point out. With this patch, if an input string ends with 
> a `-`, it won't be consumed anymore. I suppose it doesn't matter.


That looks fine. A trailing `-` looks like someone wanted to add more bytes but 
then pressed return too early, so rejecting that sounds perfectly reasonable.

Since you've obviously been looking at this code lately, I've also added you 
the to D80755 , for the mac perspective of 
accepting "uuid"s with different lengths.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80807



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

[Lldb-commits] [PATCH] D80755: Support build-ids of other sizes than 16 in UUID::SetFromStringRef

[Pavel Labath via Phabricator via lldb-commits]

labath added a reviewer: friss.
labath added inline comments.



Comment at: lldb/source/Utility/UUID.cpp:93-94
+bool UUID::SetFromStringRef(llvm::StringRef str) {
+  const size_t max_uuid_size = 20;
+  const size_t min_uuid_size = 4;
+

I don't think we should be restricting the size here in any way. It is possible 
to produce larger build-ids already (-Wl,--build-id=0xlonghexstring), and the 
rest of the UUID class does support arbitrary sizes. Some tools (e.g. 
llvm-readelf) will choke on them, but let's try to not make lldb one of those 
tools.

Using super-short uuids is most likely a bad idea, and will result in a lot of 
collisions, but if someone really wants to use a 3-byte "uuid", I don't see a 
reason to stop him here.


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

https://reviews.llvm.org/D80755



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

[Lldb-commits] [PATCH] D80112: Check if thread was suspended during previous stop added.

[Ilya Bukonkin via Phabricator via lldb-commits]

fallkrum updated this revision to Diff 267561.
fallkrum added a comment.
Herald added subscribers: dexonsmith, mgorny.

Unit tests added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80112

Files:
  lldb/unittests/CMakeLists.txt
  lldb/unittests/Thread/CMakeLists.txt
  lldb/unittests/Thread/ThreadTest.cpp

Index: lldb/unittests/Thread/ThreadTest.cpp
===
--- /dev/null
+++ lldb/unittests/Thread/ThreadTest.cpp
@@ -0,0 +1,178 @@
+//===-- ThreadTest.cpp --===//
+//
+// 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/Thread.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/Process.h"
+#include "lldb/Target/StopInfo.h"
+#include "lldb/Utility/ArchSpec.h"
+#include "lldb/Utility/Reproducer.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace lldb_private::repro;
+using namespace lldb;
+
+namespace {
+class ThreadTest : 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; }
+
+  ProcessModID &GetModIDNonConstRef() { return m_mod_id; }
+};
+
+class DummyThread : public Thread {
+public:
+  using Thread::Thread;
+
+  ~DummyThread() override { DestroyThread(); }
+
+  bool IsStillAtLastBreakpointHit() override { return true; }
+
+  void RefreshStateAfterStop() override {}
+
+  lldb::RegisterContextSP GetRegisterContext() override { return nullptr; }
+
+  lldb::RegisterContextSP
+  CreateRegisterContextForFrame(StackFrame *frame) override {
+return nullptr;
+  }
+
+  bool CalculateStopInfo() override { return false; }
+};
+} // namespace
+
+TargetSP CreateTarget(DebuggerSP &debugger_sp, ArchSpec &arch) {
+  Status error;
+  PlatformSP platform_sp;
+  TargetSP target_sp;
+  error = debugger_sp->GetTargetList().CreateTarget(
+  *debugger_sp, "", arch, eLoadDependentsNo, platform_sp, target_sp);
+
+  if (target_sp) {
+debugger_sp->GetTargetList().SetSelectedTarget(target_sp.get());
+  }
+
+  return target_sp;
+}
+
+TEST_F(ThreadTest, SetStopInfo) {
+  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 = CreateTarget(debugger_sp, arch);
+  ASSERT_TRUE(target_sp);
+
+  ListenerSP listener_sp(Listener::MakeListener("dummy"));
+  ProcessSP process_sp = std::make_shared(target_sp, listener_sp);
+  ASSERT_TRUE(process_sp);
+
+  DummyProcess *process = static_cast(process_sp.get());
+
+  ThreadSP thread_sp = std::make_shared(*process_sp.get(), 0);
+  ASSERT_TRUE(thread_sp);
+
+  StopInfoSP stopinfo_sp =
+  StopInfo::CreateStopReasonWithBreakpointSiteID(*thread_sp.get(), 0);
+  ASSERT_TRUE(stopinfo_sp->IsValid() == true);
+
+  /*
+   Should make stopinfo valid.
+   */
+  process->GetModIDNonConstRef().BumpStopID();
+  ASSERT_TRUE(stopinfo_sp->IsValid() == false);
+
+  thread_sp->SetStopInfo(stopinfo_sp);
+  ASSERT_TRUE(stopinfo_sp->IsValid() == true);
+}
+
+TEST_F(ThreadTest, GetPrivateStopInfo) {
+  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 = CreateTarget(debugger_sp, arch);
+  ASSERT_TRUE(target_sp);
+
+  ListenerSP listener_sp(Listener::MakeListener("dummy"));
+  ProcessSP p

[Lldb-commits] [PATCH] D80112: Check if thread was suspended during previous stop added.

[Ilya Bukonkin via Phabricator via lldb-commits]

fallkrum updated this revision to Diff 267564.
fallkrum added a comment.

Added changes made to Thread.cpp itself to the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80112

Files:
  lldb/source/Target/Thread.cpp
  lldb/unittests/CMakeLists.txt
  lldb/unittests/Thread/CMakeLists.txt
  lldb/unittests/Thread/ThreadTest.cpp

Index: lldb/unittests/Thread/ThreadTest.cpp
===
--- /dev/null
+++ lldb/unittests/Thread/ThreadTest.cpp
@@ -0,0 +1,178 @@
+//===-- ThreadTest.cpp --===//
+//
+// 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/Thread.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/Process.h"
+#include "lldb/Target/StopInfo.h"
+#include "lldb/Utility/ArchSpec.h"
+#include "lldb/Utility/Reproducer.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace lldb_private::repro;
+using namespace lldb;
+
+namespace {
+class ThreadTest : 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; }
+
+  ProcessModID &GetModIDNonConstRef() { return m_mod_id; }
+};
+
+class DummyThread : public Thread {
+public:
+  using Thread::Thread;
+
+  ~DummyThread() override { DestroyThread(); }
+
+  bool IsStillAtLastBreakpointHit() override { return true; }
+
+  void RefreshStateAfterStop() override {}
+
+  lldb::RegisterContextSP GetRegisterContext() override { return nullptr; }
+
+  lldb::RegisterContextSP
+  CreateRegisterContextForFrame(StackFrame *frame) override {
+return nullptr;
+  }
+
+  bool CalculateStopInfo() override { return false; }
+};
+} // namespace
+
+TargetSP CreateTarget(DebuggerSP &debugger_sp, ArchSpec &arch) {
+  Status error;
+  PlatformSP platform_sp;
+  TargetSP target_sp;
+  error = debugger_sp->GetTargetList().CreateTarget(
+  *debugger_sp, "", arch, eLoadDependentsNo, platform_sp, target_sp);
+
+  if (target_sp) {
+debugger_sp->GetTargetList().SetSelectedTarget(target_sp.get());
+  }
+
+  return target_sp;
+}
+
+TEST_F(ThreadTest, SetStopInfo) {
+  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 = CreateTarget(debugger_sp, arch);
+  ASSERT_TRUE(target_sp);
+
+  ListenerSP listener_sp(Listener::MakeListener("dummy"));
+  ProcessSP process_sp = std::make_shared(target_sp, listener_sp);
+  ASSERT_TRUE(process_sp);
+
+  DummyProcess *process = static_cast(process_sp.get());
+
+  ThreadSP thread_sp = std::make_shared(*process_sp.get(), 0);
+  ASSERT_TRUE(thread_sp);
+
+  StopInfoSP stopinfo_sp =
+  StopInfo::CreateStopReasonWithBreakpointSiteID(*thread_sp.get(), 0);
+  ASSERT_TRUE(stopinfo_sp->IsValid() == true);
+
+  /*
+   Should make stopinfo valid.
+   */
+  process->GetModIDNonConstRef().BumpStopID();
+  ASSERT_TRUE(stopinfo_sp->IsValid() == false);
+
+  thread_sp->SetStopInfo(stopinfo_sp);
+  ASSERT_TRUE(stopinfo_sp->IsValid() == true);
+}
+
+TEST_F(ThreadTest, GetPrivateStopInfo) {
+  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 = CreateTarget(debugger_sp, arch);
+  ASSERT_TRUE(target_sp);
+
+  ListenerSP listener_sp(Listener::MakeListener("du

[Lldb-commits] [lldb] 2b37c5b - [lldb][NFC] Make ClangExpressionSourceCode's wrapping logic more consistent

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2020-06-01T13:24:30+02:00
New Revision: 2b37c5b560584f05edf5d375d4ca86fe9c5b0173

URL: 
https://github.com/llvm/llvm-project/commit/2b37c5b560584f05edf5d375d4ca86fe9c5b0173
DIFF: 
https://github.com/llvm/llvm-project/commit/2b37c5b560584f05edf5d375d4ca86fe9c5b0173.diff

LOG: [lldb][NFC] Make ClangExpressionSourceCode's wrapping logic more consistent

Summary:
ClangExpressionSourceCode has different ways to wrap the user expression based 
on
which context the expression is executed in. For example, if we're in a C++ 
member
function we put the expression inside a fake member function of a fake class to 
make the
evaluation possible. Similar things are done for Objective-C instance/static 
methods.
There is also a default wrapping where we put the expression in a normal 
function
just to make it possible to execute it.

The way we currently define which kind of wrapping the expression needs is 
based on
the `wrapping_language` we keep passing to the ClangExpressionSourceCode
instance. We repurposed the language type enum for that variable to distinguish 
the
cases above with the following mapping:
* language = C_plus_plus -> member function wrapping
* language = ObjC -> instance/static method wrapping (`is_static` distinguished 
between those two).
* language = C -> normal function wrapping
* all other cases like C_plus_plus11, Haskell etc. make our class a no-op that 
does mostly nothing.

That mapping is currently not documented and just confusing as the `language`
is unrelated to the expression language (and in the ClangUserExpression we even 
pretend
that it *is* the actual language, but luckily never used it for anything). Some 
of the code
in ClangExpressionSourceCode is also obviously thinking that this is the actual 
language of
the expression as it checks for non-existent cases such as `ObjC_plus_plus` 
which is
not part of the mapping.

This patch makes a new enum to describe the four cases above (with 
instance/static Objective-C
methods now being their own case). It also make that enum just a member of
ClangExpressionSourceCode instead of having to pass the same value to the class 
repeatedly.
This gets also rid of all the switch-case-checks for 'unknown' language such as 
C_plus_plus11 as this
is no longer necessary.

Reviewers: labath, JDevlieghere

Reviewed By: labath

Subscribers: abidh

Differential Revision: https://reviews.llvm.org/D80793

Added: 


Modified: 
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h
lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h

Removed: 




diff  --git 
a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp 
b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
index 41d2b4adf3ca..a429963277d1 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
@@ -174,8 +174,8 @@ static void AddMacros(const DebugMacros *dm, CompileUnit 
*comp_unit,
 
 lldb_private::ClangExpressionSourceCode::ClangExpressionSourceCode(
 llvm::StringRef filename, llvm::StringRef name, llvm::StringRef prefix,
-llvm::StringRef body, Wrapping wrap)
-: ExpressionSourceCode(name, prefix, body, wrap) {
+llvm::StringRef body, Wrapping wrap, WrapKind wrap_kind)
+: ExpressionSourceCode(name, prefix, body, wrap), m_wrap_kind(wrap_kind) {
   // Use #line markers to pretend that we have a single-line source file
   // containing only the user expression. This will hide our wrapper code
   // from the user when we render diagnostics with Clang.
@@ -261,10 +261,9 @@ TokenVerifier::TokenVerifier(std::string body) {
   }
 }
 
-static void AddLocalVariableDecls(const lldb::VariableListSP &var_list_sp,
-  StreamString &stream,
-  const std::string &expr,
-  lldb::LanguageType wrapping_language) {
+void ClangExpressionSourceCode::AddLocalVariableDecls(
+const lldb::VariableListSP &var_list_sp, StreamString &stream,
+const std::string &expr) const {
   TokenVerifier tokens(expr);
 
   for (size_t i = 0; i < var_list_sp->GetSize(); i++) {
@@ -281,13 +280,12 @@ static void AddLocalVariableDecls(const 
lldb::VariableListSP &var_list_sp,
 if (!expr.empty() && !tokens.hasToken(var_name.GetStringRef()))
   continue;
 
-if ((var_name == "self" || var_name == "_cmd") &&
-(wrapping_language == lldb::eLanguageTypeObjC ||
- wrapping_language == lldb::eLanguageTypeObjC_plus_plus))
+const bool is_objc = m_wrap_kind == WrapKind::ObjCInstanceMethod ||
+ m_wrap_kind == WrapKind::ObjCStaticMetho

[Lldb-commits] [PATCH] D80793: [lldb][NFC] Make ClangExpressionSourceCode's wrapping logic more consistent

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2b37c5b56058: [lldb][NFC] Make 
ClangExpressionSourceCode's wrapping logic more consistent (authored by 
teemperor).
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80793

Files:
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
@@ -185,7 +185,8 @@
 ExecutionContext &exe_ctx,
 std::vector modules_to_import,
 bool for_completion);
-  void UpdateLanguageForExpr();
+  /// Defines how the current expression should be wrapped.
+  ClangExpressionSourceCode::WrapKind GetWrapKind() const;
   bool SetupPersistentState(DiagnosticManager &diagnostic_manager,
ExecutionContext &exe_ctx);
   bool PrepareForParsing(DiagnosticManager &diagnostic_manager,
@@ -208,8 +209,6 @@
 lldb::TargetSP m_target_sp;
   };
 
-  /// The language type of the current expression.
-  lldb::LanguageType m_expr_lang = lldb::eLanguageTypeUnknown;
   /// The include directories that should be used when parsing the expression.
   std::vector m_include_directories;
 
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
@@ -397,16 +397,20 @@
"current compilation unit.");
 }
 
-void ClangUserExpression::UpdateLanguageForExpr() {
-  m_expr_lang = lldb::LanguageType::eLanguageTypeUnknown;
-  if (m_options.GetExecutionPolicy() == eExecutionPolicyTopLevel)
-return;
+ClangExpressionSourceCode::WrapKind ClangUserExpression::GetWrapKind() const {
+  assert(m_options.GetExecutionPolicy() != eExecutionPolicyTopLevel &&
+ "Top level expressions aren't wrapped.");
+  using Kind = ClangExpressionSourceCode::WrapKind;
   if (m_in_cplusplus_method)
-m_expr_lang = lldb::eLanguageTypeC_plus_plus;
-  else if (m_in_objectivec_method)
-m_expr_lang = lldb::eLanguageTypeObjC;
-  else
-m_expr_lang = lldb::eLanguageTypeC;
+return Kind::CppMemberFunction;
+  else if (m_in_objectivec_method) {
+if (m_in_static_method)
+  return Kind::ObjCStaticMethod;
+return Kind::ObjCInstanceMethod;
+  }
+  // Not in any kind of 'special' function, so just wrap it in a normal C
+  // function.
+  return Kind::Function;
 }
 
 void ClangUserExpression::CreateSourceCode(
@@ -420,10 +424,9 @@
 m_transformed_text = m_expr_text;
   } else {
 m_source_code.reset(ClangExpressionSourceCode::CreateWrapped(
-m_filename, prefix, m_expr_text));
+m_filename, prefix, m_expr_text, GetWrapKind()));
 
-if (!m_source_code->GetText(m_transformed_text, m_expr_lang,
-m_in_static_method, exe_ctx, !m_ctx_obj,
+if (!m_source_code->GetText(m_transformed_text, exe_ctx, !m_ctx_obj,
 for_completion, modules_to_import)) {
   diagnostic_manager.PutString(eDiagnosticSeverityError,
"couldn't construct expression body");
@@ -435,7 +438,7 @@
 std::size_t original_start;
 std::size_t original_end;
 bool found_bounds = m_source_code->GetOriginalBodyBounds(
-m_transformed_text, m_expr_lang, original_start, original_end);
+m_transformed_text, original_start, original_end);
 if (found_bounds)
   m_user_expression_start_pos = original_start;
   }
@@ -560,7 +563,6 @@
llvm::make_range(m_include_directories.begin(),
 m_include_directories.end()));
 
-  UpdateLanguageForExpr();
   CreateSourceCode(diagnostic_manager, exe_ctx, imported_modules,
for_completion);
   return true;
@@ -635,9 +637,8 @@
 m_fixed_text = diagnostic_manager.GetFixedExpression();
 // Retrieve the original expression in case we don't have a top level
 // expression (which has no surrounding source code).
-if (m_source_code &&
-m_source_code->GetOriginalBodyBounds(m_fixed_text, m_expr_lang,
- fixed_start, fixed_end))
+if (m_source_code && m_source_code->GetOriginalBodyBounds(
+   

[Lldb-commits] [lldb] 54422d2 - Revert "[lldb] Pass -fPIC flag even when DYLIB_ONLY is set"

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2020-06-01T14:41:08+02:00
New Revision: 54422d21700cfb532c80b22662f7b79d741b21ba

URL: 
https://github.com/llvm/llvm-project/commit/54422d21700cfb532c80b22662f7b79d741b21ba
DIFF: 
https://github.com/llvm/llvm-project/commit/54422d21700cfb532c80b22662f7b79d741b21ba.diff

LOG: Revert "[lldb] Pass -fPIC flag even when DYLIB_ONLY is set"

This reverts commit fd0ab3b3eb88de3fe4792c34b50084595e22d68d.

The fix here is incorrect and the actual fault was an incorrect test Makefile.

To give some more background:

The original test for D80798 compiled three source files into either one
executable or one executable + 2 shared libraries, each being one different
test setup. If both the monolithic executable and the shared libraries
where compiled in the same directory, then Make would overwrite the .o files
of one test setup with the other. This caused that while -fPIC was passed
correctly to the test setup with the shared libraries, the compiler invocations
for the monolithic executable would later overwrite these object files (and
as only the test setup with the shared library used -fPIC, it appeared as if
the shared library object files didn't receive the -fPIC flag).

Thanks to Pavel for figuring this out.

Added: 


Modified: 
lldb/packages/Python/lldbsuite/test/make/Makefile.rules

Removed: 




diff  --git a/lldb/packages/Python/lldbsuite/test/make/Makefile.rules 
b/lldb/packages/Python/lldbsuite/test/make/Makefile.rules
index 5e3f47884990..ea0fa748bc36 100644
--- a/lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ b/lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -657,14 +657,9 @@ endif
 $(DYLIB_OBJECTS) : CFLAGS += -DCOMPILING_LLDB_TEST_DLL
 
 ifneq "$(OS)" "Windows_NT"
-ifeq "$(DYLIB_ONLY)" ""
-CFLAGS += -fPIC
-CXXFLAGS += -fPIC
-else
 $(DYLIB_OBJECTS) : CFLAGS += -fPIC
 $(DYLIB_OBJECTS) : CXXFLAGS += -fPIC
 endif
-endif
 
 $(DYLIB_FILENAME) : $(DYLIB_OBJECTS)
 ifeq "$(OS)" "Darwin"



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

[Lldb-commits] [PATCH] D80519: [lldb/DWARF] Add support for pre-standard GNU call site attributes

[Pavel Labath via Phabricator via lldb-commits]

labath marked 2 inline comments as done.
labath added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3740
+  if (tail_call)
+call_inst_pc = low_pc;
+  else

vsk wrote:
> labath wrote:
> > vsk wrote:
> > > I think this needs to be `call_inst_pc = low_pc - 1`, see 
> > > `DwarfCompileUnit::constructCallSiteEntryDIE` for the rationale, and 
> > > `StackFrameList::SynthesizeTailCallFrames` for where we use this 
> > > information. The relevant part of the comment from 
> > > SynthesizeTailCallFrames is:
> > > 
> > > "We do not want to subtract 1 from this PC, as it's the actual address of 
> > > the tail-calling branch instruction. This address is provided by the 
> > > compiler via DW_AT_call_pc."
> > > 
> > > In GNU+Dwarf4 mode, that's no longer true, the DW_AT_low_pc is a fake 
> > > "return address" for the tail call (really: the address of the 
> > > instruction after the tail-calling jump).
> > > 
> > > On x86_64, this test doesn't seem to stress this case, but the test 
> > > breaks on Darwin/arm64 without the adjustment.
> > Heh, you're right. I should've looked at what the code does instead of just 
> > trying to reverse engineer the logic from the output. I've now added the -1.
> > After looking that this code some more, I've come to realize that it's 
> > usage of the lack of DW_AT_call_return_pc to indicate a tail call is not 
> > correct -- I don't see anything preventing a producer from generating this 
> > attribute even for tail calls. I'm going to try refactoring this in another 
> > patch to store the tail-call-ness more explicitly. That should also make 
> > this part slightly cleaner.
> The parsing code uses DW_AT_call_tail_call to determine whether or not 
> there's a tail call, which is already explicit. However, the CallEdge 
> representation does take an invalid return_pc address to mean there's a tail 
> call. My $0.02 is that that's legit, and a producer that emits 
> DW_AT_call_return_pc at a tail call site is behaving badly. If we care to 
> support that, we could do it by changing the condition on line 3710 to `attr 
> == DW_AT_call_return_pc && !tail_call`.
You're right again. I confused a tail call with a (regular) call to a noreturn 
function.

The real reason why I wanted to do that change was to avoid the -1 thingy here. 
If we carried the information about a tail call and the information whether the 
address points before/after the call instruction explicitly,  I believe that 
the -1 here wouldn't be necessary, and the code in `SynthesizeTailCallFrames` 
could just set `behaves_like_frame_zero` depending on what kind of address it 
gets. I have a feeling that would be cleaner, but I have to try it out...



Comment at: 
lldb/test/API/functionalities/tail_call_frames/disambiguate_paths_to_common_sink/main.cpp:8
+  // FROM-FUNC1-NEXT: func1
+  // FROM-FUNC1-SAME: [artificial]
+  // FROM-FUNC1-NEXT: main

vsk wrote:
> Are these test updates necessary because lldb doesn't print '[opt]' and 
> '[artificial]' next to frame descriptions in a consistent way across 
> platforms? Or is it just that you don't think matching '[opt]' is relevant to 
> the test?
Right, I wanted to mention that as it's not very obvious, but I forgot...

The `[opt]` thingy is not printed at all with -ggdb because the attribute we 
get this information from -- DW_AT_APPLE_optimized -- is only emitted for 
-glldb. The optimization flag did not seem very relevant for these tests (I 
mean, technically the compiler could emit call site attributes even in 
non-optimized mode) so instead of forking the expectations I chose to simply 
remove it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80519



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

[Lldb-commits] [PATCH] D80659: [lldb-vscode] Redirect stderr and stdout to DAPs console message

[Richard Howell via Phabricator via lldb-commits]

rmaz added a comment.



In D80659#2062304 , @labath wrote:

> Btw, given that Richard has found some vscode code which already tries to 
> send stderr to the console window 
> https://github.com/microsoft/vscode/blob/master/src/vs/workbench/contrib/debug/node/debugAdapter.ts#L234,
>  it would be good to first understand why doesn't that kick in. Maybe there's 
> a vscode bug that could be fixed instead.


The issue is fairly clear, that listener is only added if the optional 
constructor arg `outputService` is defined, but it never is. I was planning on 
putting up a diff this week some time to always read from stderr regardless.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80659



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

[Lldb-commits] [PATCH] D80519: [lldb/DWARF] Add support for pre-standard GNU call site attributes

[Vedant Kumar via Phabricator via lldb-commits]

vsk accepted this revision.
vsk added a comment.
This revision is now accepted and ready to land.

Thanks, lgtm!




Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3740
+  if (tail_call)
+call_inst_pc = low_pc;
+  else

labath wrote:
> vsk wrote:
> > labath wrote:
> > > vsk wrote:
> > > > I think this needs to be `call_inst_pc = low_pc - 1`, see 
> > > > `DwarfCompileUnit::constructCallSiteEntryDIE` for the rationale, and 
> > > > `StackFrameList::SynthesizeTailCallFrames` for where we use this 
> > > > information. The relevant part of the comment from 
> > > > SynthesizeTailCallFrames is:
> > > > 
> > > > "We do not want to subtract 1 from this PC, as it's the actual address 
> > > > of the tail-calling branch instruction. This address is provided by the 
> > > > compiler via DW_AT_call_pc."
> > > > 
> > > > In GNU+Dwarf4 mode, that's no longer true, the DW_AT_low_pc is a fake 
> > > > "return address" for the tail call (really: the address of the 
> > > > instruction after the tail-calling jump).
> > > > 
> > > > On x86_64, this test doesn't seem to stress this case, but the test 
> > > > breaks on Darwin/arm64 without the adjustment.
> > > Heh, you're right. I should've looked at what the code does instead of 
> > > just trying to reverse engineer the logic from the output. I've now added 
> > > the -1.
> > > After looking that this code some more, I've come to realize that it's 
> > > usage of the lack of DW_AT_call_return_pc to indicate a tail call is not 
> > > correct -- I don't see anything preventing a producer from generating 
> > > this attribute even for tail calls. I'm going to try refactoring this in 
> > > another patch to store the tail-call-ness more explicitly. That should 
> > > also make this part slightly cleaner.
> > The parsing code uses DW_AT_call_tail_call to determine whether or not 
> > there's a tail call, which is already explicit. However, the CallEdge 
> > representation does take an invalid return_pc address to mean there's a 
> > tail call. My $0.02 is that that's legit, and a producer that emits 
> > DW_AT_call_return_pc at a tail call site is behaving badly. If we care to 
> > support that, we could do it by changing the condition on line 3710 to 
> > `attr == DW_AT_call_return_pc && !tail_call`.
> You're right again. I confused a tail call with a (regular) call to a 
> noreturn function.
> 
> The real reason why I wanted to do that change was to avoid the -1 thingy 
> here. If we carried the information about a tail call and the information 
> whether the address points before/after the call instruction explicitly,  I 
> believe that the -1 here wouldn't be necessary, and the code in 
> `SynthesizeTailCallFrames` could just set `behaves_like_frame_zero` depending 
> on what kind of address it gets. I have a feeling that would be cleaner, but 
> I have to try it out...
I think you could do this by adding a flag to the CallEdge class, like:

```
bool have_one_after_call_inst_pc = ...;
union {
  addr_t call_inst_pc;
  addr_t one_after_call_inst_pc;
};
```

and then set `behaves_like_frame_zero = 
!call_edge.have_one_after_call_inst_pc`. But I'm not convinced this 
successfully separates the concerns re: parsing DWARF vs. adjusting PC values.



Comment at: 
lldb/test/API/functionalities/tail_call_frames/disambiguate_paths_to_common_sink/main.cpp:8
+  // FROM-FUNC1-NEXT: func1
+  // FROM-FUNC1-SAME: [artificial]
+  // FROM-FUNC1-NEXT: main

labath wrote:
> vsk wrote:
> > Are these test updates necessary because lldb doesn't print '[opt]' and 
> > '[artificial]' next to frame descriptions in a consistent way across 
> > platforms? Or is it just that you don't think matching '[opt]' is relevant 
> > to the test?
> Right, I wanted to mention that as it's not very obvious, but I forgot...
> 
> The `[opt]` thingy is not printed at all with -ggdb because the attribute we 
> get this information from -- DW_AT_APPLE_optimized -- is only emitted for 
> -glldb. The optimization flag did not seem very relevant for these tests (I 
> mean, technically the compiler could emit call site attributes even in 
> non-optimized mode) so instead of forking the expectations I chose to simply 
> remove it.
Sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80519



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

[Lldb-commits] [PATCH] D80519: [lldb/DWARF] Add support for pre-standard GNU call site attributes

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a subscriber: aprantl.
dblaikie added inline comments.



Comment at: 
lldb/test/API/functionalities/tail_call_frames/disambiguate_paths_to_common_sink/main.cpp:8
+  // FROM-FUNC1-NEXT: func1
+  // FROM-FUNC1-SAME: [artificial]
+  // FROM-FUNC1-NEXT: main

vsk wrote:
> labath wrote:
> > vsk wrote:
> > > Are these test updates necessary because lldb doesn't print '[opt]' and 
> > > '[artificial]' next to frame descriptions in a consistent way across 
> > > platforms? Or is it just that you don't think matching '[opt]' is 
> > > relevant to the test?
> > Right, I wanted to mention that as it's not very obvious, but I forgot...
> > 
> > The `[opt]` thingy is not printed at all with -ggdb because the attribute 
> > we get this information from -- DW_AT_APPLE_optimized -- is only emitted 
> > for -glldb. The optimization flag did not seem very relevant for these 
> > tests (I mean, technically the compiler could emit call site attributes 
> > even in non-optimized mode) so instead of forking the expectations I chose 
> > to simply remove it.
> Sounds good.
As an aside, now that lldb understands these attributes - perhaps we should 
emit them under -glldb as well as -ggdb? (@aprantl might be interested in 
making that call)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80519



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

[Lldb-commits] [lldb] 382f6d3 - [lldb/Test] Add test for man page and lldb --help output

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-06-01T13:04:45-07:00
New Revision: 382f6d37a1f2ec472a1f869be2d33078fe6ea8da

URL: 
https://github.com/llvm/llvm-project/commit/382f6d37a1f2ec472a1f869be2d33078fe6ea8da
DIFF: 
https://github.com/llvm/llvm-project/commit/382f6d37a1f2ec472a1f869be2d33078fe6ea8da.diff

LOG: [lldb/Test] Add test for man page and lldb --help output

Added: 
lldb/test/Shell/Driver/TestHelp.test

Modified: 


Removed: 




diff  --git a/lldb/test/Shell/Driver/TestHelp.test 
b/lldb/test/Shell/Driver/TestHelp.test
new file mode 100644
index ..fc53e80bdc4d
--- /dev/null
+++ b/lldb/test/Shell/Driver/TestHelp.test
@@ -0,0 +1,67 @@
+UNSUPPORTED: lldb-repro
+
+RUN: %lldb --help | FileCheck %s
+RUN: cat %S/../../../docs/man/lldb.rst | FileCheck %s
+
+CHECK: ATTACHING
+CHECK: --attach-name
+CHECK: --attach-pid
+CHECK: -n 
+CHECK: -p 
+CHECK: --wait-for
+CHECK: -w
+
+CHECK: COMMANDS
+CHECK: --batch
+CHECK: -b
+CHECK: -K 
+CHECK: -k 
+CHECK: --local-lldbinit
+CHECK: --no-lldbinit
+CHECK: --one-line-before-file
+CHECK: --one-line-on-crash
+CHECK: --one-line
+CHECK: -O
+CHECK: -o
+CHECK: -Q
+CHECK: --source-before-file
+CHECK: --source-on-crash
+CHECK: --source-quietly
+CHECK: --source
+CHECK: -S
+CHECK: -s
+CHECK: -x
+
+CHECK: OPTIONS
+CHECK: --arch
+CHECK: -a
+CHECK: --capture-path
+CHECK: --capture
+CHECK: --core
+CHECK: -c
+CHECK: --debug
+CHECK: -d
+CHECK: --editor
+CHECK: -e
+CHECK: --file
+CHECK: -f
+CHECK: --help
+CHECK: -h
+CHECK: --no-use-colors
+CHECK: --replay
+CHECK: --version
+CHECK: -v
+CHECK: -X
+
+CHECK: REPL
+CHECK: -r
+CHECK: --repl-language
+CHECK: --repl
+CHECK: -R
+CHECK: -r
+
+CHECK: SCRIPTING
+CHECK: -l
+CHECK: --python-path
+CHECK: -P
+CHECK: --script-language



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

[Lldb-commits] [PATCH] D80955: Fix UB in EmulateInstructionARM64.cpp

[Adrian Prantl via Phabricator via lldb-commits]

aprantl created this revision.
aprantl added reviewers: jasonmolenda, vsk.
Herald added subscribers: danielkiss, kristof.beyls.

This fixes an unhandled signed integer overflow in AddWithCarry() by using the 
llvm::checkedAdd() function. Thats to @vsk for the suggestion!


https://reviews.llvm.org/D80955

Files:
  lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp


Index: lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
===
--- lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
+++ lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
@@ -8,8 +8,6 @@
 
 #include "EmulateInstructionARM64.h"
 
-#include 
-
 #include "lldb/Core/Address.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Symbol/UnwindPlan.h"
@@ -18,10 +16,14 @@
 #include "lldb/Utility/RegisterValue.h"
 #include "lldb/Utility/Stream.h"
 
+#include "llvm/Support/CheckedArithmetic.h"
+
 #include "Plugins/Process/Utility/ARMDefines.h"
 #include "Plugins/Process/Utility/ARMUtils.h"
 #include "Plugins/Process/Utility/lldb-arm64-register-enums.h"
 
+#include 
+
 #define GPR_OFFSET(idx) ((idx)*8)
 #define GPR_OFFSET_NAME(reg) 0
 #define FPU_OFFSET(idx) ((idx)*16)
@@ -91,14 +93,17 @@
 AddWithCarry(uint32_t N, uint64_t x, uint64_t y, bit carry_in,
  EmulateInstructionARM64::ProcState &proc_state) {
   uint64_t unsigned_sum = UInt(x) + UInt(y) + UInt(carry_in);
-  int64_t signed_sum = SInt(x) + SInt(y) + UInt(carry_in);
+  llvm::Optional signed_sum = llvm::checkedAdd(SInt(x), SInt(y));
+  bool overflow = !signed_sum;
+  if (!overflow)
+overflow |= !llvm::checkedAdd(*signed_sum, SInt(carry_in));
   uint64_t result = unsigned_sum;
   if (N < 64)
 result = Bits64(result, N - 1, 0);
   proc_state.N = Bit64(result, N - 1);
   proc_state.Z = IsZero(result);
   proc_state.C = UInt(result) == unsigned_sum;
-  proc_state.V = SInt(result) == signed_sum;
+  proc_state.V = !overflow;
   return result;
 }
 


Index: lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
===
--- lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
+++ lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
@@ -8,8 +8,6 @@
 
 #include "EmulateInstructionARM64.h"
 
-#include 
-
 #include "lldb/Core/Address.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Symbol/UnwindPlan.h"
@@ -18,10 +16,14 @@
 #include "lldb/Utility/RegisterValue.h"
 #include "lldb/Utility/Stream.h"
 
+#include "llvm/Support/CheckedArithmetic.h"
+
 #include "Plugins/Process/Utility/ARMDefines.h"
 #include "Plugins/Process/Utility/ARMUtils.h"
 #include "Plugins/Process/Utility/lldb-arm64-register-enums.h"
 
+#include 
+
 #define GPR_OFFSET(idx) ((idx)*8)
 #define GPR_OFFSET_NAME(reg) 0
 #define FPU_OFFSET(idx) ((idx)*16)
@@ -91,14 +93,17 @@
 AddWithCarry(uint32_t N, uint64_t x, uint64_t y, bit carry_in,
  EmulateInstructionARM64::ProcState &proc_state) {
   uint64_t unsigned_sum = UInt(x) + UInt(y) + UInt(carry_in);
-  int64_t signed_sum = SInt(x) + SInt(y) + UInt(carry_in);
+  llvm::Optional signed_sum = llvm::checkedAdd(SInt(x), SInt(y));
+  bool overflow = !signed_sum;
+  if (!overflow)
+overflow |= !llvm::checkedAdd(*signed_sum, SInt(carry_in));
   uint64_t result = unsigned_sum;
   if (N < 64)
 result = Bits64(result, N - 1, 0);
   proc_state.N = Bit64(result, N - 1);
   proc_state.Z = IsZero(result);
   proc_state.C = UInt(result) == unsigned_sum;
-  proc_state.V = SInt(result) == signed_sum;
+  proc_state.V = !overflow;
   return result;
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D80755: Support build-ids of other sizes than 16 in UUID::SetFromStringRef

[Jaroslav Sevcik via Phabricator via lldb-commits]

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



Comment at: lldb/source/Utility/UUID.cpp:109
 *this = fromData(bytes);
-return str.size() - rest.size();
+return str.size();
   }

jankratochvil wrote:
> Now the return type could be `bool`.
I was worried about having to touch even more call sites, but perhaps it is not 
too bad.



Comment at: lldb/source/Utility/UUID.cpp:93-94
+bool UUID::SetFromStringRef(llvm::StringRef str) {
+  const size_t max_uuid_size = 20;
+  const size_t min_uuid_size = 4;
+

labath wrote:
> I don't think we should be restricting the size here in any way. It is 
> possible to produce larger build-ids already 
> (-Wl,--build-id=0xlonghexstring), and the rest of the UUID class does support 
> arbitrary sizes. Some tools (e.g. llvm-readelf) will choke on them, but let's 
> try to not make lldb one of those tools.
> 
> Using super-short uuids is most likely a bad idea, and will result in a lot 
> of collisions, but if someone really wants to use a 3-byte "uuid", I don't 
> see a reason to stop him here.
Good idea, that makes the code even simpler.


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

https://reviews.llvm.org/D80755



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

[Lldb-commits] [PATCH] D80755: Support build-ids of other sizes than 16 in UUID::SetFromStringRef

[Jaroslav Sevcik via Phabricator via lldb-commits]

jarin updated this revision to Diff 267739.
jarin edited the summary of this revision.
jarin added a comment.

Removed size restrictions on UUIDs.


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

https://reviews.llvm.org/D80755

Files:
  lldb/include/lldb/Utility/UUID.h
  lldb/source/Interpreter/OptionValueUUID.cpp
  lldb/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
  lldb/source/Plugins/Process/MacOSX-Kernel/CommunicationKDP.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Utility/UUID.cpp
  lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
  
lldb/test/API/functionalities/postmortem/minidump-new/linux-arm-uuids-elf-build-id-4.yaml
  
lldb/test/API/functionalities/postmortem/minidump-new/linux-arm-uuids-match.yaml
  lldb/unittests/ObjectFile/ELF/TestObjectFileELF.cpp
  lldb/unittests/Target/ModuleCacheTest.cpp
  lldb/unittests/Utility/UUIDTest.cpp

Index: lldb/unittests/Utility/UUIDTest.cpp
===
--- lldb/unittests/Utility/UUIDTest.cpp
+++ lldb/unittests/Utility/UUIDTest.cpp
@@ -45,7 +45,7 @@
   from_str.SetFromStringRef("----");
   UUID opt_from_str;
   opt_from_str.SetFromOptionalStringRef("----");
-  
+
   EXPECT_FALSE(empty);
   EXPECT_TRUE(a16);
   EXPECT_TRUE(a20);
@@ -57,25 +57,28 @@
 
 TEST(UUIDTest, SetFromStringRef) {
   UUID u;
-  EXPECT_EQ(32u, u.SetFromStringRef("404142434445464748494a4b4c4d4e4f"));
+  EXPECT_TRUE(u.SetFromStringRef("404142434445464748494a4b4c4d4e4f"));
   EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNO", 16), u);
 
-  EXPECT_EQ(36u, u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f"));
+  EXPECT_TRUE(u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f"));
   EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNO", 16), u);
 
-  EXPECT_EQ(45u, u.SetFromStringRef(
- "40-41-42-43-4445464748494a4b4c4d4e4f-50515253", 20));
+  EXPECT_TRUE(
+  u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f-50515253"));
   EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNOPQRS", 20), u);
 
-  EXPECT_EQ(0u, u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f", 20));
-  EXPECT_EQ(0u, u.SetFromStringRef("40x"));
-  EXPECT_EQ(0u, u.SetFromStringRef(""));
-  EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNOPQRS", 20), u)
+  EXPECT_TRUE(u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f"));
+
+  EXPECT_FALSE(u.SetFromStringRef("40x"));
+  EXPECT_FALSE(u.SetFromStringRef(""));
+  EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNO", 16), u)
   << "uuid was changed by failed parse calls";
 
-  EXPECT_EQ(
-  32u, u.SetFromStringRef("404142434445464748494a4b4c4d4e4f-50515253", 16));
-  EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNO", 16), u);
+  EXPECT_TRUE(u.SetFromStringRef("404142434445464748494a4b4c4d4e4f-50515253"));
+  EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNOPQRS", 20), u);
+
+  EXPECT_TRUE(u.SetFromStringRef("40414243"));
+  EXPECT_EQ(UUID::fromData("@ABCD", 4), u);
 }
 
 TEST(UUIDTest, StringConverion) {
Index: lldb/unittests/Target/ModuleCacheTest.cpp
===
--- lldb/unittests/Target/ModuleCacheTest.cpp
+++ lldb/unittests/Target/ModuleCacheTest.cpp
@@ -41,7 +41,6 @@
 static const char module_name[] = "TestModule.so";
 static const char module_uuid[] =
 "F4E7E991-9B61-6AD4-0073-561AC3D9FA10-C043A476";
-static const uint32_t uuid_bytes = 20;
 static const size_t module_size = 5602;
 
 static FileSpec GetDummyRemotePath() {
@@ -87,7 +86,7 @@
   ModuleCache mc;
   ModuleSpec module_spec;
   module_spec.GetFileSpec() = GetDummyRemotePath();
-  module_spec.GetUUID().SetFromStringRef(module_uuid, uuid_bytes);
+  module_spec.GetUUID().SetFromStringRef(module_uuid);
   module_spec.SetObjectSize(module_size);
   ModuleSP module_sp;
   bool did_create;
Index: lldb/unittests/ObjectFile/ELF/TestObjectFileELF.cpp
===
--- lldb/unittests/ObjectFile/ELF/TestObjectFileELF.cpp
+++ lldb/unittests/ObjectFile/ELF/TestObjectFileELF.cpp
@@ -156,7 +156,7 @@
   ModuleSpec Spec;
   ASSERT_TRUE(Specs.GetModuleSpecAtIndex(0, Spec)) ;
   UUID Uuid;
-  Uuid.SetFromStringRef("1b8a73ac238390e32a7ff4ac8ebe4d6a41ecf5c9", 20);
+  Uuid.SetFromStringRef("1b8a73ac238390e32a7ff4ac8ebe4d6a41ecf5c9");
   EXPECT_EQ(Spec.GetUUID(), Uuid);
 }
 
@@ -284,4 +284,4 @@
 
   auto entry_point_addr = module_sp->GetObjectFile()->GetEntryPointAddress();
   ASSERT_EQ(entry_point_addr.GetAddressClass(), AddressClass::eCode);
-}
\ No newline at end of file
+}
Index: lldb/test/API/functionalities/postmortem/minidump-new/linux-arm-uuids-match.yaml
===
--- /dev/null
+++ lldb/test/API/functionalities/postmortem/minidump-new/linux-arm-uuids-match.yaml
@@ -0,0 +1,15 @@
+--- !minidump
+Streams: 
+  - Type:Sy

[Lldb-commits] [PATCH] D80955: Fix UB in EmulateInstructionARM64.cpp

[Vedant Kumar via Phabricator via lldb-commits]

vsk added a comment.

Any chance we can export AddWithCarry via EmulateInstructionARM64.h, then add a 
unit test in TestArm64InstEmulation.cpp?


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

https://reviews.llvm.org/D80955



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

[Lldb-commits] [PATCH] D80955: Fix UB in EmulateInstructionARM64.cpp

[Vedant Kumar via Phabricator via lldb-commits]

vsk added a comment.

I should add: otherwise, this looks good!




Comment at: lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp:99
+  if (!overflow)
+overflow |= !llvm::checkedAdd(*signed_sum, SInt(carry_in));
   uint64_t result = unsigned_sum;

The docs [1] say 'integer signed_sum = SInt(x) + SInt(y) + UInt(carry_in)', but 
I bet checkedAdd doesn't support that, and anyway carry_in is 1-bit so it 
doesn't matter.

[1] 
https://developer.arm.com/docs/ddi0596/e/shared-pseudocode-functions/shared-functionsinteger-pseudocode#impl-shared.AddWithCarry.3


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

https://reviews.llvm.org/D80955



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

[Lldb-commits] [PATCH] D80955: Fix UB in EmulateInstructionARM64.cpp

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Nice cleanup of a long standing undefined behavior warning.


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

https://reviews.llvm.org/D80955



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

[Lldb-commits] [PATCH] D80955: Fix UB in EmulateInstructionARM64.cpp

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.



Comment at: lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp:25
 
+#include 
+

Why? also should be `cstdlib` in C++.


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

https://reviews.llvm.org/D80955



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

[Lldb-commits] [PATCH] D80755: Support build-ids of other sizes than 16 in UUID::SetFromStringRef

[Jaroslav Sevcik via Phabricator via lldb-commits]

jarin updated this revision to Diff 267754.
jarin added a comment.

Added a test for missing nibble in UUID.


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

https://reviews.llvm.org/D80755

Files:
  lldb/include/lldb/Utility/UUID.h
  lldb/source/Interpreter/OptionValueUUID.cpp
  lldb/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
  lldb/source/Plugins/Process/MacOSX-Kernel/CommunicationKDP.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Utility/UUID.cpp
  lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
  
lldb/test/API/functionalities/postmortem/minidump-new/linux-arm-uuids-elf-build-id-4.yaml
  
lldb/test/API/functionalities/postmortem/minidump-new/linux-arm-uuids-match.yaml
  lldb/unittests/ObjectFile/ELF/TestObjectFileELF.cpp
  lldb/unittests/Target/ModuleCacheTest.cpp
  lldb/unittests/Utility/UUIDTest.cpp

Index: lldb/unittests/Utility/UUIDTest.cpp
===
--- lldb/unittests/Utility/UUIDTest.cpp
+++ lldb/unittests/Utility/UUIDTest.cpp
@@ -45,7 +45,7 @@
   from_str.SetFromStringRef("----");
   UUID opt_from_str;
   opt_from_str.SetFromOptionalStringRef("----");
-  
+
   EXPECT_FALSE(empty);
   EXPECT_TRUE(a16);
   EXPECT_TRUE(a20);
@@ -57,25 +57,30 @@
 
 TEST(UUIDTest, SetFromStringRef) {
   UUID u;
-  EXPECT_EQ(32u, u.SetFromStringRef("404142434445464748494a4b4c4d4e4f"));
+  EXPECT_TRUE(u.SetFromStringRef("404142434445464748494a4b4c4d4e4f"));
   EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNO", 16), u);
 
-  EXPECT_EQ(36u, u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f"));
+  EXPECT_TRUE(u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f"));
   EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNO", 16), u);
 
-  EXPECT_EQ(45u, u.SetFromStringRef(
- "40-41-42-43-4445464748494a4b4c4d4e4f-50515253", 20));
+  EXPECT_TRUE(
+  u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f-50515253"));
   EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNOPQRS", 20), u);
 
-  EXPECT_EQ(0u, u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f", 20));
-  EXPECT_EQ(0u, u.SetFromStringRef("40x"));
-  EXPECT_EQ(0u, u.SetFromStringRef(""));
-  EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNOPQRS", 20), u)
+  EXPECT_TRUE(u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f"));
+
+  EXPECT_FALSE(u.SetFromStringRef("40x"));
+  EXPECT_FALSE(u.SetFromStringRef(""));
+  EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNO", 16), u)
   << "uuid was changed by failed parse calls";
 
-  EXPECT_EQ(
-  32u, u.SetFromStringRef("404142434445464748494a4b4c4d4e4f-50515253", 16));
-  EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNO", 16), u);
+  EXPECT_TRUE(u.SetFromStringRef("404142434445464748494a4b4c4d4e4f-50515253"));
+  EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNOPQRS", 20), u);
+
+  EXPECT_TRUE(u.SetFromStringRef("40414243"));
+  EXPECT_EQ(UUID::fromData("@ABCD", 4), u);
+
+  EXPECT_FALSE(u.SetFromStringRef("4"));
 }
 
 TEST(UUIDTest, StringConverion) {
Index: lldb/unittests/Target/ModuleCacheTest.cpp
===
--- lldb/unittests/Target/ModuleCacheTest.cpp
+++ lldb/unittests/Target/ModuleCacheTest.cpp
@@ -41,7 +41,6 @@
 static const char module_name[] = "TestModule.so";
 static const char module_uuid[] =
 "F4E7E991-9B61-6AD4-0073-561AC3D9FA10-C043A476";
-static const uint32_t uuid_bytes = 20;
 static const size_t module_size = 5602;
 
 static FileSpec GetDummyRemotePath() {
@@ -87,7 +86,7 @@
   ModuleCache mc;
   ModuleSpec module_spec;
   module_spec.GetFileSpec() = GetDummyRemotePath();
-  module_spec.GetUUID().SetFromStringRef(module_uuid, uuid_bytes);
+  module_spec.GetUUID().SetFromStringRef(module_uuid);
   module_spec.SetObjectSize(module_size);
   ModuleSP module_sp;
   bool did_create;
Index: lldb/unittests/ObjectFile/ELF/TestObjectFileELF.cpp
===
--- lldb/unittests/ObjectFile/ELF/TestObjectFileELF.cpp
+++ lldb/unittests/ObjectFile/ELF/TestObjectFileELF.cpp
@@ -156,7 +156,7 @@
   ModuleSpec Spec;
   ASSERT_TRUE(Specs.GetModuleSpecAtIndex(0, Spec)) ;
   UUID Uuid;
-  Uuid.SetFromStringRef("1b8a73ac238390e32a7ff4ac8ebe4d6a41ecf5c9", 20);
+  Uuid.SetFromStringRef("1b8a73ac238390e32a7ff4ac8ebe4d6a41ecf5c9");
   EXPECT_EQ(Spec.GetUUID(), Uuid);
 }
 
@@ -284,4 +284,4 @@
 
   auto entry_point_addr = module_sp->GetObjectFile()->GetEntryPointAddress();
   ASSERT_EQ(entry_point_addr.GetAddressClass(), AddressClass::eCode);
-}
\ No newline at end of file
+}
Index: lldb/test/API/functionalities/postmortem/minidump-new/linux-arm-uuids-match.yaml
===
--- /dev/null
+++ lldb/test/API/functionalities/postmortem/minidump-new/linux-arm-uuids-match.yaml
@@ -0,0 +1,15 @@
+--- !minidump
+Streams: 
+  - Type:

[Lldb-commits] [PATCH] D80955: Fix UB in EmulateInstructionARM64.cpp

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

This is great Adrian. I feel like there are several other instances of 
additions that should use `llvm::checkAdd` here instead of `+`. Is this the 
only UB triggered in the testsuite?


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

https://reviews.llvm.org/D80955



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

[Lldb-commits] [PATCH] D80955: Fix UB in EmulateInstructionARM64.cpp

[Davide Italiano via Phabricator via lldb-commits]

davide added inline comments.



Comment at: lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp:25
 
+#include 
+

shafik wrote:
> Why? also should be `cstdlib` in C++.
It's just something shuffled around, probably a side effect of clang-format.


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

https://reviews.llvm.org/D80955



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

[Lldb-commits] [PATCH] D80955: Fix UB in EmulateInstructionARM64.cpp

[Adrian Prantl via Phabricator via lldb-commits]

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



Comment at: lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp:99
+  if (!overflow)
+overflow |= !llvm::checkedAdd(*signed_sum, SInt(carry_in));
   uint64_t result = unsigned_sum;

vsk wrote:
> The docs [1] say 'integer signed_sum = SInt(x) + SInt(y) + UInt(carry_in)', 
> but I bet checkedAdd doesn't support that, and anyway carry_in is 1-bit so it 
> doesn't matter.
> 
> [1] 
> https://developer.arm.com/docs/ddi0596/e/shared-pseudocode-functions/shared-functionsinteger-pseudocode#impl-shared.AddWithCarry.3
Am I misreading this or are we also setting the C(arry) flag inverted according 
to the documentation?


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

https://reviews.llvm.org/D80955



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

[Lldb-commits] [lldb] a0b674f - Fix UB in EmulateInstructionARM64.cpp

[Adrian Prantl via lldb-commits]

Author: Adrian Prantl
Date: 2020-06-01T18:11:50-07:00
New Revision: a0b674fd7f06b86241cf19387313b508248a3868

URL: 
https://github.com/llvm/llvm-project/commit/a0b674fd7f06b86241cf19387313b508248a3868
DIFF: 
https://github.com/llvm/llvm-project/commit/a0b674fd7f06b86241cf19387313b508248a3868.diff

LOG: Fix UB in EmulateInstructionARM64.cpp

This fixes an unhandled signed integer overflow in AddWithCarry() by
using the llvm::checkedAdd() function. Thats to Vedant Kumar for the
suggestion!



Differential Revision: https://reviews.llvm.org/D80955

Added: 
lldb/unittests/Instruction/CMakeLists.txt
lldb/unittests/Instruction/TestAArch64Emulator.cpp

Modified: 
lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.h
lldb/unittests/CMakeLists.txt

Removed: 




diff  --git a/lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp 
b/lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
index dcf41444e48f..35fb51759ca8 100644
--- a/lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
+++ b/lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
@@ -8,8 +8,6 @@
 
 #include "EmulateInstructionARM64.h"
 
-#include 
-
 #include "lldb/Core/Address.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Symbol/UnwindPlan.h"
@@ -18,10 +16,14 @@
 #include "lldb/Utility/RegisterValue.h"
 #include "lldb/Utility/Stream.h"
 
+#include "llvm/Support/CheckedArithmetic.h"
+
 #include "Plugins/Process/Utility/ARMDefines.h"
 #include "Plugins/Process/Utility/ARMUtils.h"
 #include "Plugins/Process/Utility/lldb-arm64-register-enums.h"
 
+#include 
+
 #define GPR_OFFSET(idx) ((idx)*8)
 #define GPR_OFFSET_NAME(reg) 0
 #define FPU_OFFSET(idx) ((idx)*16)
@@ -85,23 +87,6 @@ static inline uint64_t LSL(uint64_t x, integer shift) {
   return x << shift;
 }
 
-// AddWithCarry()
-// ===
-static inline uint64_t
-AddWithCarry(uint32_t N, uint64_t x, uint64_t y, bit carry_in,
- EmulateInstructionARM64::ProcState &proc_state) {
-  uint64_t unsigned_sum = UInt(x) + UInt(y) + UInt(carry_in);
-  int64_t signed_sum = SInt(x) + SInt(y) + UInt(carry_in);
-  uint64_t result = unsigned_sum;
-  if (N < 64)
-result = Bits64(result, N - 1, 0);
-  proc_state.N = Bit64(result, N - 1);
-  proc_state.Z = IsZero(result);
-  proc_state.C = UInt(result) == unsigned_sum;
-  proc_state.V = SInt(result) == signed_sum;
-  return result;
-}
-
 // ConstrainUnpredictable()
 // 
 
@@ -582,6 +567,24 @@ bool EmulateInstructionARM64::ConditionHolds(const 
uint32_t cond) {
   return result;
 }
 
+uint64_t EmulateInstructionARM64::
+AddWithCarry(uint32_t N, uint64_t x, uint64_t y, bit carry_in,
+ EmulateInstructionARM64::ProcState &proc_state) {
+  uint64_t unsigned_sum = UInt(x) + UInt(y) + UInt(carry_in);
+  llvm::Optional signed_sum = llvm::checkedAdd(SInt(x), SInt(y));
+  bool overflow = !signed_sum;
+  if (!overflow)
+overflow |= !llvm::checkedAdd(*signed_sum, SInt(carry_in));
+  uint64_t result = unsigned_sum;
+  if (N < 64)
+result = Bits64(result, N - 1, 0);
+  proc_state.N = Bit64(result, N - 1);
+  proc_state.Z = IsZero(result);
+  proc_state.C = UInt(result) != unsigned_sum;
+  proc_state.V = overflow;
+  return result;
+}
+
 bool EmulateInstructionARM64::EmulateADDSUBImm(const uint32_t opcode) {
   // integer d = UInt(Rd);
   // integer n = UInt(Rn);

diff  --git a/lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.h 
b/lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.h
index 4841e813d89e..11ad8a99b0fc 100644
--- a/lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.h
+++ b/lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.h
@@ -152,6 +152,9 @@ class EmulateInstructionARM64 : public 
lldb_private::EmulateInstruction {
   } ProcState;
 
 protected:
+  static uint64_t AddWithCarry(uint32_t N, uint64_t x, uint64_t y, bool 
carry_in,
+   EmulateInstructionARM64::ProcState &proc_state);
+
   typedef struct {
 uint32_t mask;
 uint32_t value;

diff  --git a/lldb/unittests/CMakeLists.txt b/lldb/unittests/CMakeLists.txt
index 42aa2c2d7567..33818b5888c0 100644
--- a/lldb/unittests/CMakeLists.txt
+++ b/lldb/unittests/CMakeLists.txt
@@ -71,6 +71,7 @@ add_subdirectory(Editline)
 add_subdirectory(Expression)
 add_subdirectory(Host)
 add_subdirectory(Interpreter)
+add_subdirectory(Instruction)
 add_subdirectory(Language)
 add_subdirectory(ObjectFile)
 add_subdirectory(Platform)

diff  --git a/lldb/unittests/Instruction/CMakeLists.txt 
b/lldb/unittests/Instruction/CMakeLists.txt
new file mode 100644
index ..63d829831023
--- /dev/null
+++ b/lldb/unittests/Instruction/CMakeLists.txt
@@ -0,0 +1,12 @@
+if("ARM" IN_LIST LLVM_TARGETS_TO_BUILD)
+  add_lldb_unittest(EmulatorTests
+TestAArch64Emulator

[Lldb-commits] [PATCH] D80955: Fix UB in EmulateInstructionARM64.cpp

[Adrian Prantl via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa0b674fd7f06: Fix UB in EmulateInstructionARM64.cpp 
(authored by aprantl).
Herald added a subscriber: mgorny.
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D80955?vs=267734&id=267776#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80955

Files:
  lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
  lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.h
  lldb/unittests/CMakeLists.txt
  lldb/unittests/Instruction/CMakeLists.txt
  lldb/unittests/Instruction/TestAArch64Emulator.cpp

Index: lldb/unittests/Instruction/TestAArch64Emulator.cpp
===
--- /dev/null
+++ lldb/unittests/Instruction/TestAArch64Emulator.cpp
@@ -0,0 +1,62 @@
+//===-- TestAArch64Emulator.cpp --===//
+
+//
+// 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 "gtest/gtest.h"
+
+#include "lldb/Core/Address.h"
+#include "lldb/Core/Disassembler.h"
+#include "lldb/Target/ExecutionContext.h"
+#include "lldb/Utility/ArchSpec.h"
+
+#include "Plugins/Instruction/ARM64/EmulateInstructionARM64.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+struct Arch64EmulatorTester : public EmulateInstructionARM64 {
+  Arch64EmulatorTester()
+  : EmulateInstructionARM64(ArchSpec("arm64-apple-ios")) {}
+
+  static uint64_t AddWithCarry(uint32_t N, uint64_t x, uint64_t y, bool carry_in,
+   EmulateInstructionARM64::ProcState &proc_state) {
+return EmulateInstructionARM64::AddWithCarry(N, x, y, carry_in, proc_state);
+  }
+};
+
+class TestAArch64Emulator : public testing::Test {
+public:
+  static void SetUpTestCase();
+  static void TearDownTestCase();
+
+protected:
+};
+
+void TestAArch64Emulator::SetUpTestCase() {
+  EmulateInstructionARM64::Initialize();
+}
+
+void TestAArch64Emulator::TearDownTestCase() {
+  EmulateInstructionARM64::Terminate();
+}
+
+TEST_F(TestAArch64Emulator, TestOverflow) {
+  EmulateInstructionARM64::ProcState pstate;
+  memset(&pstate, 0, sizeof(pstate));
+  uint64_t ll_max = std::numeric_limits::max();
+  Arch64EmulatorTester emu;
+  ASSERT_EQ(emu.AddWithCarry(64, ll_max, 0, 0, pstate), ll_max);
+  ASSERT_EQ(pstate.V, 0ULL);
+  ASSERT_EQ(pstate.C, 0ULL);
+  ASSERT_EQ(emu.AddWithCarry(64, ll_max, 1, 0, pstate), (uint64_t)(ll_max + 1));
+  ASSERT_EQ(pstate.V, 1ULL);
+  ASSERT_EQ(pstate.C, 0ULL);
+  ASSERT_EQ(emu.AddWithCarry(64, ll_max, 0, 1, pstate), (uint64_t)(ll_max + 1));
+  ASSERT_EQ(pstate.V, 1ULL);
+  ASSERT_EQ(pstate.C, 0ULL);
+}
Index: lldb/unittests/Instruction/CMakeLists.txt
===
--- /dev/null
+++ lldb/unittests/Instruction/CMakeLists.txt
@@ -0,0 +1,12 @@
+if("ARM" IN_LIST LLVM_TARGETS_TO_BUILD)
+  add_lldb_unittest(EmulatorTests
+TestAArch64Emulator.cpp
+LINK_LIBS
+  lldbCore
+  lldbSymbol
+  lldbTarget
+  lldbPluginInstructionARM64
+LINK_COMPONENTS
+  Support
+  ${LLVM_TARGETS_TO_BUILD})
+endif()
Index: lldb/unittests/CMakeLists.txt
===
--- lldb/unittests/CMakeLists.txt
+++ lldb/unittests/CMakeLists.txt
@@ -71,6 +71,7 @@
 add_subdirectory(Expression)
 add_subdirectory(Host)
 add_subdirectory(Interpreter)
+add_subdirectory(Instruction)
 add_subdirectory(Language)
 add_subdirectory(ObjectFile)
 add_subdirectory(Platform)
Index: lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.h
===
--- lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.h
+++ lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.h
@@ -152,6 +152,9 @@
   } ProcState;
 
 protected:
+  static uint64_t AddWithCarry(uint32_t N, uint64_t x, uint64_t y, bool carry_in,
+   EmulateInstructionARM64::ProcState &proc_state);
+
   typedef struct {
 uint32_t mask;
 uint32_t value;
Index: lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
===
--- lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
+++ lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
@@ -8,8 +8,6 @@
 
 #include "EmulateInstructionARM64.h"
 
-#include 
-
 #include "lldb/Core/Address.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Symbol/UnwindPlan.h"
@@ -18,10 +16,14 @@
 #include "lldb/Utility/R

[Lldb-commits] [PATCH] D80955: Fix UB in EmulateInstructionARM64.cpp

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

> This is great Adrian. I feel like there are several other instances of 
> additions that should use llvm::checkAdd here instead of +. Is this the only 
> UB triggered in the testsuite?

It's the only UB caught by the green dragon / ci.swift.org bots.


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

https://reviews.llvm.org/D80955



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

[Lldb-commits] [PATCH] D80793: [lldb][NFC] Make ClangExpressionSourceCode's wrapping logic more consistent

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.



Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h:33-40
+/// Wrapped in a member function of a C++ class.
+CppMemberFunction,
+/// Wrapped in a instance Objective-C method.
+ObjCInstanceMethod,
+/// Wrapped in a static Objective-C method.
+ObjCStaticMethod,
+/// Wrapped in a non-member function.

labath wrote:
> If I understand the code correctly, a c++ static member function is wrapped 
> using the "function" approach. I think this is potentially confusing, so it 
> would be good if the comments elaborated on it more (e.g. add "non-static" to 
> the CppMemberFunction description, and explicitly mention static member 
> functions in the "function" description).
It might be worth adding that we don't need to find the `this` for the C++ case 
unlike a non-static member function. This happens in `LookUpLldbClass`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80793



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

[Lldb-commits] [PATCH] D80793: [lldb][NFC] Make ClangExpressionSourceCode's wrapping logic more consistent

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.



Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h:41
+/// Note that this is also used for static member functions of a C++ class.
+Function
+  };

I would actually feel better have a separate enumerator for C++ static member 
functions and just having it fall-through when used. It would be 
self-documenting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80793



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