[Lldb-commits] [lldb] b65d4b2 - [lldb/DWARF] Look for complete array element definitions in other modules

2020-07-06 Thread Pavel Labath via lldb-commits 

Author: Pavel Labath
Date: 2020-07-06T10:09:13+02:00
New Revision: b65d4b23f6dd4da4277acbf2bb912becce4f8a57

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

LOG: [lldb/DWARF] Look for complete array element definitions in other modules

This applies the same logic we have for incomplete class bases and
members to array element types.

Added: 


Modified: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
lldb/test/API/functionalities/limit-debug-info/TestLimitDebugInfo.py
lldb/test/API/functionalities/limit-debug-info/main.cpp
lldb/test/API/functionalities/limit-debug-info/one.cpp
lldb/test/API/functionalities/limit-debug-info/onetwo.h
lldb/test/API/functionalities/limit-debug-info/two.cpp

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 0bb69eb91362..0bd2d0c05c1b 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -1267,32 +1267,20 @@ TypeSP DWARFASTParserClang::ParseArrayType(const 
DWARFDIE &die,
   if (TypeSystemClang::IsCXXClassType(array_element_type) &&
   !array_element_type.GetCompleteType()) {
 ModuleSP module_sp = die.GetModule();
-if (module_sp) {
-  if (die.GetCU()->GetProducer() == eProducerClang)
-module_sp->ReportError(
-"DWARF DW_TAG_array_type DIE at 0x%8.8x has a "
-"class/union/struct element type DIE 0x%8.8x that is a "
-"forward declaration, not a complete definition.\nTry "
-"compiling the source file with -fstandalone-debug or "
-"disable -gmodules",
-die.GetOffset(), type_die.GetOffset());
-  else
-module_sp->ReportError(
-"DWARF DW_TAG_array_type DIE at 0x%8.8x has a "
-"class/union/struct element type DIE 0x%8.8x that is a "
-"forward declaration, not a complete definition.\nPlease "
-"file a bug against the compiler and include the "
-"preprocessed output for %s",
-die.GetOffset(), type_die.GetOffset(), GetUnitName(die).c_str());
-}
-
-// We have no choice other than to pretend that the element class
-// type is complete. If we don't do this, clang will crash when
-// trying to layout the class. Since we provide layout
-// assistance, all ivars in this class and other classes will be
-// fine, this is the best we can do short of crashing.
+
+// Mark the class as complete, but we make a note of the fact that
+// this class is not _really_ complete so we can later search for a
+// definition in a 
diff erent module.
+// Since we provide layout assistance, all ivars in this class and other
+// classes will be fine even if we are not able to find the definition
+// elsewhere.
 if (TypeSystemClang::StartTagDeclarationDefinition(array_element_type)) {
   TypeSystemClang::CompleteTagDeclarationDefinition(array_element_type);
+  const auto *td =
+  TypeSystemClang::GetQualType(array_element_type.GetOpaqueQualType())
+  .getTypePtr()
+  ->getAsTagDecl();
+  m_ast.GetMetadata(td)->SetIsForcefullyCompleted();
 } else {
   module_sp->ReportError("DWARF DIE at 0x%8.8x was not able to "
  "start its definition.\nPlease file a "
@@ -2741,7 +2729,7 @@ void DWARFASTParserClang::ParseSingleMember(
 
   if (TypeSystemClang::IsCXXClassType(member_clang_type) &&
   !member_clang_type.GetCompleteType()) {
-// Mark the class as complete, ut we make a note of the fact that
+// Mark the class as complete, but we make a note of the fact that
 // this class is not _really_ complete so we can later search for a
 // definition in a 
diff erent module.
 // Since we provide layout assistance, all ivars in this class and

diff  --git 
a/lldb/test/API/functionalities/limit-debug-info/TestLimitDebugInfo.py 
b/lldb/test/API/functionalities/limit-debug-info/TestLimitDebugInfo.py
index 396861f5eb76..9408ad6eee1d 100644
--- a/lldb/test/API/functionalities/limit-debug-info/TestLimitDebugInfo.py
+++ b/lldb/test/API/functionalities/limit-debug-info/TestLimitDebugInfo.py
@@ -54,6 +54,10 @@ def test_one_and_two_debug(self):
 self.expect_expr("two_as_member.two.one.member", result_value="147")
 self.expect_expr("two_as_member.two.member", result_value="247")
 
+self.expect_expr("array_of_one[2].member", result_value="174")
+self.expect_expr("array_of_two[2].one[2].member", result_value="174")
+self.expect_expr("array_of_two[2].member", res

[Lldb-commits] [lldb] 5daa39a - [lldb/Utility] Merge Scalar::Get(Value)TypeAsCString

2020-07-06 Thread Pavel Labath via lldb-commits 

Author: Pavel Labath
Date: 2020-07-06T10:34:12+02:00
New Revision: 5daa39aa4c355e899c2ceb371ba3c8347200a687

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

LOG: [lldb/Utility] Merge Scalar::Get(Value)TypeAsCString

Added: 


Modified: 
lldb/include/lldb/Utility/Scalar.h
lldb/source/Utility/Scalar.cpp
lldb/unittests/Utility/ScalarTest.cpp

Removed: 




diff  --git a/lldb/include/lldb/Utility/Scalar.h 
b/lldb/include/lldb/Utility/Scalar.h
index 275df4d63b0d..f215fa71c84c 100644
--- a/lldb/include/lldb/Utility/Scalar.h
+++ b/lldb/include/lldb/Utility/Scalar.h
@@ -125,7 +125,7 @@ class Scalar {
 m_integer.clearAllBits();
   }
 
-  const char *GetTypeAsCString() const;
+  const char *GetTypeAsCString() const { return GetValueTypeAsCString(m_type); 
}
 
   void GetValue(Stream *s, bool show_type) const;
 

diff  --git a/lldb/source/Utility/Scalar.cpp b/lldb/source/Utility/Scalar.cpp
index c36ccab21a39..7397744fb51c 100644
--- a/lldb/source/Utility/Scalar.cpp
+++ b/lldb/source/Utility/Scalar.cpp
@@ -229,44 +229,6 @@ void Scalar::GetValue(Stream *s, bool show_type) const {
   }
 }
 
-const char *Scalar::GetTypeAsCString() const {
-  switch (m_type) {
-  case e_void:
-return "void";
-  case e_sint:
-return "int";
-  case e_uint:
-return "unsigned int";
-  case e_slong:
-return "long";
-  case e_ulong:
-return "unsigned long";
-  case e_slonglong:
-return "long long";
-  case e_ulonglong:
-return "unsigned long long";
-  case e_sint128:
-return "int128_t";
-  case e_uint128:
-return "unsigned int128_t";
-  case e_sint256:
-return "int256_t";
-  case e_uint256:
-return "unsigned int256_t";
-  case e_sint512:
-return "int512_t";
-  case e_uint512:
-return "unsigned int512_t";
-  case e_float:
-return "float";
-  case e_double:
-return "double";
-  case e_long_double:
-return "long double";
-  }
-  return "";
-}
-
 Scalar::~Scalar() = default;
 
 Scalar::Type Scalar::GetBestTypeForBitSize(size_t bit_size, bool sign) {

diff  --git a/lldb/unittests/Utility/ScalarTest.cpp 
b/lldb/unittests/Utility/ScalarTest.cpp
index afbb76103ca6..910ff173bc92 100644
--- a/lldb/unittests/Utility/ScalarTest.cpp
+++ b/lldb/unittests/Utility/ScalarTest.cpp
@@ -362,7 +362,7 @@ TEST(ScalarTest, Scalar_512) {
 
   ASSERT_TRUE(S.MakeUnsigned());
   EXPECT_EQ(S.GetType(), Scalar::e_uint512);
-  ASSERT_STREQ(S.GetTypeAsCString(), "unsigned int512_t");
+  ASSERT_STREQ(S.GetTypeAsCString(), "uint512_t");
   ASSERT_STREQ(S.GetValueTypeAsCString(Scalar::e_uint512), "uint512_t");
   EXPECT_EQ(S.GetByteSize(), 64U);
 



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

[Lldb-commits] [lldb] 5814255 - [lldb] Always round down in NSDate's formatter to match NSDate's builtin format

2020-07-06 Thread Raphael Isemann via lldb-commits 

Author: Raphael Isemann
Date: 2020-07-06T16:59:37+02:00
New Revision: 5814255e1a7d2e90580d6df457ddd13b1cd156cb

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

LOG: [lldb] Always round down in NSDate's formatter to match NSDate's builtin 
format

Summary:

When printing an NSDate (for example with `NSLog` or `po`) the seconds value is
always rounded down. LLDB's own formatter however isn't following that behaviour
which leads to situations where the formatted result is sometimes one second
off. For example:

```
(lldb) p [NSDate dateWithTimeIntervalSince1970:0.1]
(__NSTaggedDate *) $1 = [...] 1970-01-01 00:00:01 UTC
(lldb) po [NSDate dateWithTimeIntervalSince1970:0.1]
1970-01-01 00:00:00 +

(lldb) p [NSDate dateWithTimeIntervalSince1970:0.6]
(__NSTaggedDate *) $4 =[...] 1970-01-01 00:00:01 UTC
(lldb) po [NSDate dateWithTimeIntervalSince1970:0.6]
1970-01-01 00:00:00 +
```

This patch just always rounds down the seconds value we get from the NSDate
object.

Fixes rdar://65084800

Reviewers: mib, davide

Reviewed By: mib

Subscribers: JDevlieghere

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

Added: 


Modified: 
lldb/source/Plugins/Language/ObjC/Cocoa.cpp

lldb/test/API/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSDate.py
lldb/test/API/functionalities/data-formatter/data-formatter-objc/main.m

Removed: 




diff  --git a/lldb/source/Plugins/Language/ObjC/Cocoa.cpp 
b/lldb/source/Plugins/Language/ObjC/Cocoa.cpp
index 8a44811dd36b..da910f48e59a 100644
--- a/lldb/source/Plugins/Language/ObjC/Cocoa.cpp
+++ b/lldb/source/Plugins/Language/ObjC/Cocoa.cpp
@@ -867,7 +867,7 @@ bool lldb_private::formatters::NSDateSummaryProvider(
   // is generally true and POSIXly happy, but might break if a library vendor
   // decides to get creative
   time_t epoch = GetOSXEpoch();
-  epoch = epoch + (time_t)date_value;
+  epoch = epoch + static_cast(std::floor(date_value));
   tm *tm_date = gmtime(&epoch);
   if (!tm_date)
 return false;

diff  --git 
a/lldb/test/API/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSDate.py
 
b/lldb/test/API/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSDate.py
index 5cfaa892bb62..61394c05f5d5 100644
--- 
a/lldb/test/API/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSDate.py
+++ 
b/lldb/test/API/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSDate.py
@@ -43,6 +43,15 @@ def nsdate_data_formatter_commands(self):
 self.expect('frame variable date4_abs', substrs=['1970'])
 self.expect('frame variable date5_abs', substrs=[now_year])
 
+# Check that LLDB always follow's NSDate's rounding behavior (which
+# is always rounding down).
+self.expect_expr("date_1970_minus_06", result_summary="1969-12-31 
23:59:59 UTC")
+self.expect_expr("date_1970_minus_05", result_summary="1969-12-31 
23:59:59 UTC")
+self.expect_expr("date_1970_minus_04", result_summary="1969-12-31 
23:59:59 UTC")
+self.expect_expr("date_1970_plus_06", result_summary="1970-01-01 
00:00:00 UTC")
+self.expect_expr("date_1970_plus_05", result_summary="1970-01-01 
00:00:00 UTC")
+self.expect_expr("date_1970_plus_04", result_summary="1970-01-01 
00:00:00 UTC")
+
 self.expect('frame variable cupertino home europe',
 substrs=['@"America/Los_Angeles"',
  '@"Europe/Rome"',

diff  --git 
a/lldb/test/API/functionalities/data-formatter/data-formatter-objc/main.m 
b/lldb/test/API/functionalities/data-formatter/data-formatter-objc/main.m
index df96a5e59b5b..a44a7837f771 100644
--- a/lldb/test/API/functionalities/data-formatter/data-formatter-objc/main.m
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-objc/main.m
@@ -655,6 +655,14 @@ int main(int argc, const char *argv[]) {
   [NSDate dateWithTimeIntervalSinceReferenceDate:
   floor([[NSDate date] timeIntervalSinceReferenceDate])];
 
+  NSDate *date_1970_minus_06 = [NSDate dateWithTimeIntervalSince1970:-0.6];
+  NSDate *date_1970_minus_05 = [NSDate dateWithTimeIntervalSince1970:-0.5];
+  NSDate *date_1970_minus_04 = [NSDate dateWithTimeIntervalSince1970:-0.4];
+
+  NSDate *date_1970_plus_06 = [NSDate dateWithTimeIntervalSince1970:0.6];
+  NSDate *date_1970_plus_05 = [NSDate dateWithTimeIntervalSince1970:0.5];
+  NSDate *date_1970_plus_04 = [NSDate dateWithTimeIntervalSince1970:0.4];
+
   CFAbsoluteTime date1_abs = CFDateGetAbsoluteTime(date1);
   CFAbsoluteTime date2_abs = CFDateGetAbsoluteTime(date2);
   CFAbsoluteTime date3_abs = CFDateGetAbsoluteTime(date3);



___
lldb-commits mailing lis

[Lldb-commits] [PATCH] D83199: [lldb/DWARF] Add a utility function for (forceful) completion of types

2020-07-06 Thread Pavel Labath via Phabricator via lldb-commits 
labath created this revision.
labath added reviewers: teemperor, aprantl.
Herald added a reviewer: shafik.
Herald added a project: LLDB.

Unify the code for requiring a complete type and move it into a single
place. The only functional change is that the "cannot start a definition
of an incomplete type" is upgrated from a runtime error/warning to an
lldbassert. An plain assert might also be fine, since (AFAICT) this can
only happen in case of a programmer error.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D83199

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h

Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
@@ -219,6 +219,12 @@
   ParsedDWARFTypeAttributes &attrs);
   lldb::TypeSP ParsePointerToMemberType(const DWARFDIE &die,
 const ParsedDWARFTypeAttributes &attrs);
+
+  /// Complete a type from debug info, or mark it as forcefully completed if
+  /// there is no of the type in the current Module. Call this function in
+  /// contexts where the usual C++ rules require a type to be complete (base
+  /// class, member, etc.).
+  void CompleteType(lldb_private::CompilerType type);
 };
 
 /// Parsed form of all attributes that are relevant for type reconstruction.
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -1263,32 +1263,7 @@
   if (attrs.byte_stride == 0 && attrs.bit_stride == 0)
 attrs.byte_stride = element_type->GetByteSize().getValueOr(0);
   CompilerType array_element_type = element_type->GetForwardCompilerType();
-
-  if (TypeSystemClang::IsCXXClassType(array_element_type) &&
-  !array_element_type.GetCompleteType()) {
-ModuleSP module_sp = die.GetModule();
-
-// Mark the class as complete, but we make a note of the fact that
-// this class is not _really_ complete so we can later search for a
-// definition in a different module.
-// Since we provide layout assistance, all ivars in this class and other
-// classes will be fine even if we are not able to find the definition
-// elsewhere.
-if (TypeSystemClang::StartTagDeclarationDefinition(array_element_type)) {
-  TypeSystemClang::CompleteTagDeclarationDefinition(array_element_type);
-  const auto *td =
-  TypeSystemClang::GetQualType(array_element_type.GetOpaqueQualType())
-  .getTypePtr()
-  ->getAsTagDecl();
-  m_ast.GetMetadata(td)->SetIsForcefullyCompleted();
-} else {
-  module_sp->ReportError("DWARF DIE at 0x%8.8x was not able to "
- "start its definition.\nPlease file a "
- "bug and attach the file at the start "
- "of this error message",
- type_die.GetOffset());
-}
-  }
+  CompleteType(array_element_type);
 
   uint64_t array_element_bit_stride =
   attrs.byte_stride * 8 + attrs.bit_stride;
@@ -1343,6 +1318,30 @@
   return nullptr;
 }
 
+void DWARFASTParserClang::CompleteType(CompilerType type) {
+  // Technically, enums can be incomplete too, but we don't handle those as they
+  // are emitted even under -flimit-debug-info.
+  if (!TypeSystemClang::IsCXXClassType(type))
+return;
+
+  if (type.GetCompleteType())
+return;
+
+  // No complete definition in this module.  Mark the class as complete to
+  // satisfy local ast invariants, but make a note of the fact that
+  // it is not _really_ complete so we can later search for a definition in a
+  // different module.
+  // Since we provide layout assistance, layouts of types containing this class
+  // will be correct even if we  are not able to find the definition elsewhere.
+  bool started = TypeSystemClang::StartTagDeclarationDefinition(type);
+  lldbassert(started && "Unable to start a class type definition.");
+  TypeSystemClang::CompleteTagDeclarationDefinition(type);
+  const auto *td = TypeSystemClang::GetQualType(type.GetOpaqueQualType())
+   .getTypePtr()
+   ->getAsTagDecl();
+  m_ast.GetMetadata(td)->SetIsForcefullyCompleted();
+}
+
 TypeSP DWARFASTParserClang::UpdateSymbolContextScopeForType(
 const SymbolContext &sc, const DWARFDIE &die, TypeSP type_sp) {
   if (!type_sp)
@@ -2045,26 +2044,8 @@
   for (const auto &base_class : bases) {
 clang::TypeSourceInfo *type_source_info =
 base_class->getTypeSourceInfo();
-if (type_source_info) {
-  CompilerType base_class_type =
-

[Lldb-commits] [PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

2020-07-06 Thread Raphael Isemann via Phabricator via lldb-commits 
teemperor added a comment.

In D83008#2131796 , @tschuett wrote:

> You could try:
>
>   clangxx_host -Xclang -fdump-record-layouts %p/Inputs/layout.cpp -emit-llvm 
> -o /dev/null
>
>
> You could commit the record layouts without your fix to show the differences 
> that you patch makes.


Yeah that's I think the proper way to check the results, but IIRC the problem 
with testing this in Clang is that `foverride-record-layout=` (the testing 
approach for this code) doesn't support specifying base class offset), so we 
can't even *trigger* the bug itself in Clang itself. That what I was referring 
to with "layout overwrite JSON file" in my comment above.


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

https://reviews.llvm.org/D83008



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

[Lldb-commits] [PATCH] D83221: [lldb] Always round down in NSDate's formatter to match NSDate's builtin format

2020-07-06 Thread Raphael Isemann via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG5814255e1a7d: [lldb] Always round down in NSDate's 
formatter to match NSDate's builtin format (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/D83221/new/

https://reviews.llvm.org/D83221

Files:
  lldb/source/Plugins/Language/ObjC/Cocoa.cpp
  
lldb/test/API/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSDate.py
  lldb/test/API/functionalities/data-formatter/data-formatter-objc/main.m


Index: lldb/test/API/functionalities/data-formatter/data-formatter-objc/main.m
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-objc/main.m
+++ lldb/test/API/functionalities/data-formatter/data-formatter-objc/main.m
@@ -655,6 +655,14 @@
   [NSDate dateWithTimeIntervalSinceReferenceDate:
   floor([[NSDate date] timeIntervalSinceReferenceDate])];
 
+  NSDate *date_1970_minus_06 = [NSDate dateWithTimeIntervalSince1970:-0.6];
+  NSDate *date_1970_minus_05 = [NSDate dateWithTimeIntervalSince1970:-0.5];
+  NSDate *date_1970_minus_04 = [NSDate dateWithTimeIntervalSince1970:-0.4];
+
+  NSDate *date_1970_plus_06 = [NSDate dateWithTimeIntervalSince1970:0.6];
+  NSDate *date_1970_plus_05 = [NSDate dateWithTimeIntervalSince1970:0.5];
+  NSDate *date_1970_plus_04 = [NSDate dateWithTimeIntervalSince1970:0.4];
+
   CFAbsoluteTime date1_abs = CFDateGetAbsoluteTime(date1);
   CFAbsoluteTime date2_abs = CFDateGetAbsoluteTime(date2);
   CFAbsoluteTime date3_abs = CFDateGetAbsoluteTime(date3);
Index: 
lldb/test/API/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSDate.py
===
--- 
lldb/test/API/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSDate.py
+++ 
lldb/test/API/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSDate.py
@@ -43,6 +43,15 @@
 self.expect('frame variable date4_abs', substrs=['1970'])
 self.expect('frame variable date5_abs', substrs=[now_year])
 
+# Check that LLDB always follow's NSDate's rounding behavior (which
+# is always rounding down).
+self.expect_expr("date_1970_minus_06", result_summary="1969-12-31 
23:59:59 UTC")
+self.expect_expr("date_1970_minus_05", result_summary="1969-12-31 
23:59:59 UTC")
+self.expect_expr("date_1970_minus_04", result_summary="1969-12-31 
23:59:59 UTC")
+self.expect_expr("date_1970_plus_06", result_summary="1970-01-01 
00:00:00 UTC")
+self.expect_expr("date_1970_plus_05", result_summary="1970-01-01 
00:00:00 UTC")
+self.expect_expr("date_1970_plus_04", result_summary="1970-01-01 
00:00:00 UTC")
+
 self.expect('frame variable cupertino home europe',
 substrs=['@"America/Los_Angeles"',
  '@"Europe/Rome"',
Index: lldb/source/Plugins/Language/ObjC/Cocoa.cpp
===
--- lldb/source/Plugins/Language/ObjC/Cocoa.cpp
+++ lldb/source/Plugins/Language/ObjC/Cocoa.cpp
@@ -867,7 +867,7 @@
   // is generally true and POSIXly happy, but might break if a library vendor
   // decides to get creative
   time_t epoch = GetOSXEpoch();
-  epoch = epoch + (time_t)date_value;
+  epoch = epoch + static_cast(std::floor(date_value));
   tm *tm_date = gmtime(&epoch);
   if (!tm_date)
 return false;


Index: lldb/test/API/functionalities/data-formatter/data-formatter-objc/main.m
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-objc/main.m
+++ lldb/test/API/functionalities/data-formatter/data-formatter-objc/main.m
@@ -655,6 +655,14 @@
   [NSDate dateWithTimeIntervalSinceReferenceDate:
   floor([[NSDate date] timeIntervalSinceReferenceDate])];
 
+  NSDate *date_1970_minus_06 = [NSDate dateWithTimeIntervalSince1970:-0.6];
+  NSDate *date_1970_minus_05 = [NSDate dateWithTimeIntervalSince1970:-0.5];
+  NSDate *date_1970_minus_04 = [NSDate dateWithTimeIntervalSince1970:-0.4];
+
+  NSDate *date_1970_plus_06 = [NSDate dateWithTimeIntervalSince1970:0.6];
+  NSDate *date_1970_plus_05 = [NSDate dateWithTimeIntervalSince1970:0.5];
+  NSDate *date_1970_plus_04 = [NSDate dateWithTimeIntervalSince1970:0.4];
+
   CFAbsoluteTime date1_abs = CFDateGetAbsoluteTime(date1);
   CFAbsoluteTime date2_abs = CFDateGetAbsoluteTime(date2);
   CFAbsoluteTime date3_abs = CFDateGetAbsoluteTime(date3);
Index: lldb/test/API/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSDate.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSDat

[Lldb-commits] [PATCH] D83180: Set generic error in SBError SetErrorToGenericError

2020-07-06 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere added a comment.

I suspect this was a copy/paste mistake. Could you add a small test for this? 
Other than that this LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83180



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

[Lldb-commits] [PATCH] D83072: [lldb-vscode] Add Compile Unit List to Modules View

2020-07-06 Thread Adrian Prantl via Phabricator via lldb-commits 
aprantl added a comment.

The patch and the description don't seem to match. Deleting the commented-out 
code is obviously fine, but I'm somewhat confused by this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83072



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

[Lldb-commits] [lldb] 60c07fd - Use CMAKE_OSX_SYSROOT instead of the environment variable SYSROOT

2020-07-06 Thread Adrian Prantl via lldb-commits 

Author: Adrian Prantl
Date: 2020-07-06T13:17:31-07:00
New Revision: 60c07fd016a3a5f2050828f92257e1e5d33a485b

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

LOG: Use CMAKE_OSX_SYSROOT instead of the environment variable SYSROOT

to detect energy support in debugserver.  The way that Swift
build-script is invoked the former may be overridden manually.



Added: 


Modified: 
lldb/tools/debugserver/source/CMakeLists.txt

Removed: 




diff  --git a/lldb/tools/debugserver/source/CMakeLists.txt 
b/lldb/tools/debugserver/source/CMakeLists.txt
index 6c4fa026092b..9a7e2eb9a1a0 100644
--- a/lldb/tools/debugserver/source/CMakeLists.txt
+++ b/lldb/tools/debugserver/source/CMakeLists.txt
@@ -129,7 +129,7 @@ if(LLDB_USE_ENTITLEMENTS)
   endif()
 endif()
 
-if($ENV{SDKROOT} MATCHES ".Internal.sdk$")
+if(${CMAKE_OSX_SYSROOT} MATCHES ".Internal.sdk$")
   message(STATUS "LLDB debugserver energy support is enabled")
   add_definitions(-DLLDB_ENERGY)
   set(ENERGY_LIBRARY -lpmenergy -lpmsample)



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

[Lldb-commits] [PATCH] D83256: [lldb] Assert on unaligned load in DataExtractor

2020-07-06 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere created this revision.
JDevlieghere added reviewers: labath, clayborg.

Somehow UBSan would only report the unaligned load in `TestLinuxCore.py` when 
running the tests with reproducers. This patch adds an assert for the 
`GetDouble` and `GetFloat` method, which triggers regardless of UBSan.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D83256

Files:
  lldb/include/lldb/Utility/DataExtractor.h
  lldb/source/Utility/DataExtractor.cpp

Index: lldb/source/Utility/DataExtractor.cpp
===
--- lldb/source/Utility/DataExtractor.cpp
+++ lldb/source/Utility/DataExtractor.cpp
@@ -15,7 +15,6 @@
 
 #include "lldb/Utility/DataBuffer.h"
 #include "lldb/Utility/DataBufferHeap.h"
-#include "lldb/Utility/Endian.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Stream.h"
@@ -624,41 +623,11 @@
 }
 
 float DataExtractor::GetFloat(offset_t *offset_ptr) const {
-  typedef float float_type;
-  float_type val = 0.0;
-  const size_t src_size = sizeof(float_type);
-  const float_type *src =
-  static_cast(GetData(offset_ptr, src_size));
-  if (src) {
-if (m_byte_order != endian::InlHostByteOrder()) {
-  const uint8_t *src_data = reinterpret_cast(src);
-  uint8_t *dst_data = reinterpret_cast(&val);
-  for (size_t i = 0; i < sizeof(float_type); ++i)
-dst_data[sizeof(float_type) - 1 - i] = src_data[i];
-} else {
-  val = *src;
-}
-  }
-  return val;
+  return Get(offset_ptr);
 }
 
 double DataExtractor::GetDouble(offset_t *offset_ptr) const {
-  typedef double float_type;
-  float_type val = 0.0;
-  const size_t src_size = sizeof(float_type);
-  const float_type *src =
-  static_cast(GetData(offset_ptr, src_size));
-  if (src) {
-if (m_byte_order != endian::InlHostByteOrder()) {
-  const uint8_t *src_data = reinterpret_cast(src);
-  uint8_t *dst_data = reinterpret_cast(&val);
-  for (size_t i = 0; i < sizeof(float_type); ++i)
-dst_data[sizeof(float_type) - 1 - i] = src_data[i];
-} else {
-  val = *src;
-}
-  }
-  return val;
+  return Get(offset_ptr);
 }
 
 long double DataExtractor::GetLongDouble(offset_t *offset_ptr) const {
Index: lldb/include/lldb/Utility/DataExtractor.h
===
--- lldb/include/lldb/Utility/DataExtractor.h
+++ lldb/include/lldb/Utility/DataExtractor.h
@@ -9,6 +9,7 @@
 #ifndef LLDB_UTILITY_DATAEXTRACTOR_H
 #define LLDB_UTILITY_DATAEXTRACTOR_H
 
+#include "lldb/Utility/Endian.h"
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-forward.h"
@@ -20,6 +21,11 @@
 #include 
 #include 
 
+#ifndef __builtin_is_aligned
+#define __builtin_is_aligned(POINTER, BYTE_COUNT)  \
+  (((uintptr_t)(const void *)(POINTER)) % (BYTE_COUNT) == 0)
+#endif
+
 namespace lldb_private {
 class Log;
 class Stream;
@@ -979,6 +985,28 @@
   }
 
 protected:
+  template  T Get(lldb::offset_t *offset_ptr) const {
+constexpr size_t src_size = sizeof(T);
+
+const T *src = static_cast(GetData(offset_ptr, src_size));
+
+if (!src)
+  return 0.0;
+
+assert(__builtin_is_aligned(src, alignof(T)));
+
+if (m_byte_order != endian::InlHostByteOrder()) {
+  T val = 0.0;
+  uint8_t *dst_data = reinterpret_cast(&val);
+  const uint8_t *src_data = reinterpret_cast(src);
+  for (size_t i = 0; i < src_size; ++i)
+dst_data[src_size - 1 - i] = src_data[i];
+  return val;
+}
+
+return *src;
+  }
+
   // Member variables
   const uint8_t *m_start; ///< A pointer to the first byte of data.
   const uint8_t
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D83256: [lldb] Assert on unaligned load in DataExtractor

2020-07-06 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere added a comment.

Pavel, would you mind taking a look at the unaligned load in `TestLinuxCore.py`?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D83256



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

[Lldb-commits] [PATCH] D83256: [lldb] Assert on unaligned load in DataExtractor

2020-07-06 Thread Shafik Yaghmour via Phabricator via lldb-commits 
shafik added inline comments.



Comment at: lldb/include/lldb/Utility/DataExtractor.h:24
 
+#ifndef __builtin_is_aligned
+#define __builtin_is_aligned(POINTER, BYTE_COUNT)  
\

What platforms are we not expecting to have `__builtin_is_aligned` on?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D83256



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

[Lldb-commits] [PATCH] D83256: [lldb] Assert on unaligned load in DataExtractor

2020-07-06 Thread Shafik Yaghmour via Phabricator via lldb-commits 
shafik added inline comments.



Comment at: lldb/include/lldb/Utility/DataExtractor.h:24
 
+#ifndef __builtin_is_aligned
+#define __builtin_is_aligned(POINTER, BYTE_COUNT)  
\

shafik wrote:
> What platforms are we not expecting to have `__builtin_is_aligned` on?
nvm, I just realized this is coming from clang not llvm.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D83256



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

[Lldb-commits] [PATCH] D83199: [lldb/DWARF] Add a utility function for (forceful) completion of types

2020-07-06 Thread Shafik Yaghmour via Phabricator via lldb-commits 
shafik accepted this revision.
shafik added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1339
+  TypeSystemClang::CompleteTagDeclarationDefinition(type);
+  const auto *td = TypeSystemClang::GetQualType(type.GetOpaqueQualType())
+   .getTypePtr()

This has to be a `TagDecl` so why use `auto`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83199



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

[Lldb-commits] [PATCH] D83199: [lldb/DWARF] Add a utility function for (forceful) completion of types

2020-07-06 Thread Raphael Isemann via Phabricator via lldb-commits 
teemperor accepted this revision.
teemperor added a comment.

LGTM too, thanks!




Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1339
+  TypeSystemClang::CompleteTagDeclarationDefinition(type);
+  const auto *td = TypeSystemClang::GetQualType(type.GetOpaqueQualType())
+   .getTypePtr()

shafik wrote:
> This has to be a `TagDecl` so why use `auto`?
`const TagDecl *td = ClangUtil::GetAsTagDecl(type);`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83199



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

[Lldb-commits] [PATCH] D83256: [lldb] Assert on unaligned load in DataExtractor

2020-07-06 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg added a comment.

There is no requirement that data be aligned when stored in a buffer that 
DataExtractor points to, so this patch doesn't seem correct. Since GetData() 
calls can return unaligned pointers, the usual way to make this work is to to 
make a local variable and do a memcpy into that local variable. See the 
ReadSwapInt32() function to see how it is done for integers. Then we need to 
swap the bytes depending on the byte size. So maybe a function that can do byte 
swapping for any sized data can be made. Then we can call it from the Get 
function.  More comments inline.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D83256



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

[Lldb-commits] [PATCH] D83256: [lldb] Assert on unaligned load in DataExtractor

2020-07-06 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.



Comment at: lldb/include/lldb/Utility/DataExtractor.h:24-28
+#ifndef __builtin_is_aligned
+#define __builtin_is_aligned(POINTER, BYTE_COUNT)  
\
+  (((uintptr_t)(const void *)(POINTER)) % (BYTE_COUNT) == 0)
+#endif
+

shafik wrote:
> shafik wrote:
> > What platforms are we not expecting to have `__builtin_is_aligned` on?
> nvm, I just realized this is coming from clang not llvm.
Remove this because we don't require alignment for anything in a DataExtractor.



Comment at: lldb/include/lldb/Utility/DataExtractor.h:988
 protected:
+  template  T Get(lldb::offset_t *offset_ptr) const {
+constexpr size_t src_size = sizeof(T);

We should probably pass in the fail value if we want this to work for more than 
floats? 
```
template  T Get(lldb::offset_t *offset_ptr, const T fail_value) 
const {
```



Comment at: lldb/include/lldb/Utility/DataExtractor.h:989
+  template  T Get(lldb::offset_t *offset_ptr) const {
+constexpr size_t src_size = sizeof(T);
+

Make a local "T" here:
```
T val = fail_value;
constexpr size_t src_size = sizeof(T);
```



Comment at: lldb/include/lldb/Utility/DataExtractor.h:996
+
+assert(__builtin_is_aligned(src, alignof(T)));
+

remove this, no alignment is required.



Comment at: lldb/include/lldb/Utility/DataExtractor.h:997
+assert(__builtin_is_aligned(src, alignof(T)));
+
+if (m_byte_order != endian::InlHostByteOrder()) {

Add a memcpy call:

```
memcpy(&val, src, src_size);
```



Comment at: lldb/include/lldb/Utility/DataExtractor.h:999
+if (m_byte_order != endian::InlHostByteOrder()) {
+  T val = 0.0;
+  uint8_t *dst_data = reinterpret_cast(&val);

remove since "val" was created above



Comment at: lldb/include/lldb/Utility/DataExtractor.h:1000-1004
+  uint8_t *dst_data = reinterpret_cast(&val);
+  const uint8_t *src_data = reinterpret_cast(src);
+  for (size_t i = 0; i < src_size; ++i)
+dst_data[src_size - 1 - i] = src_data[i];
+  return val;

Might be nice to place this into a method function that swaps data of any size. 
Then other functions can re-use it if needed.



Comment at: lldb/include/lldb/Utility/DataExtractor.h:1004
+dst_data[src_size - 1 - i] = src_data[i];
+  return val;
+}

return "return val;" here



Comment at: lldb/include/lldb/Utility/DataExtractor.h:1007
+
+return *src;
+  }

We can't rely on alignment, so return our local T value:
```
return val;
```


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D83256



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

[Lldb-commits] [PATCH] D83072: [lldb-vscode] Add Compile Unit List to Modules View

2020-07-06 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg added a comment.

When removing out commented code, you don't need a patch for that. I think you 
might have committed two things to your local branch and when you ran "arc 
diff" it only submitted the second commit. When doing "arc diff" make sure to 
use:

  git commit --amend ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83072



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

[Lldb-commits] [PATCH] D83256: [lldb] Assert on unaligned load in DataExtractor

2020-07-06 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere updated this revision to Diff 275862.
JDevlieghere marked 9 inline comments as done.
JDevlieghere added a comment.

Thanks for the clarifications Greg!


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

https://reviews.llvm.org/D83256

Files:
  lldb/include/lldb/Utility/DataExtractor.h
  lldb/source/Utility/DataExtractor.cpp


Index: lldb/source/Utility/DataExtractor.cpp
===
--- lldb/source/Utility/DataExtractor.cpp
+++ lldb/source/Utility/DataExtractor.cpp
@@ -15,7 +15,6 @@
 
 #include "lldb/Utility/DataBuffer.h"
 #include "lldb/Utility/DataBufferHeap.h"
-#include "lldb/Utility/Endian.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Stream.h"
@@ -624,41 +623,11 @@
 }
 
 float DataExtractor::GetFloat(offset_t *offset_ptr) const {
-  typedef float float_type;
-  float_type val = 0.0;
-  const size_t src_size = sizeof(float_type);
-  const float_type *src =
-  static_cast(GetData(offset_ptr, src_size));
-  if (src) {
-if (m_byte_order != endian::InlHostByteOrder()) {
-  const uint8_t *src_data = reinterpret_cast(src);
-  uint8_t *dst_data = reinterpret_cast(&val);
-  for (size_t i = 0; i < sizeof(float_type); ++i)
-dst_data[sizeof(float_type) - 1 - i] = src_data[i];
-} else {
-  val = *src;
-}
-  }
-  return val;
+  return Get(offset_ptr, 0.0);
 }
 
 double DataExtractor::GetDouble(offset_t *offset_ptr) const {
-  typedef double float_type;
-  float_type val = 0.0;
-  const size_t src_size = sizeof(float_type);
-  const float_type *src =
-  static_cast(GetData(offset_ptr, src_size));
-  if (src) {
-if (m_byte_order != endian::InlHostByteOrder()) {
-  const uint8_t *src_data = reinterpret_cast(src);
-  uint8_t *dst_data = reinterpret_cast(&val);
-  for (size_t i = 0; i < sizeof(float_type); ++i)
-dst_data[sizeof(float_type) - 1 - i] = src_data[i];
-} else {
-  val = *src;
-}
-  }
-  return val;
+  return Get(offset_ptr, 0.0);
 }
 
 long double DataExtractor::GetLongDouble(offset_t *offset_ptr) const {
Index: lldb/include/lldb/Utility/DataExtractor.h
===
--- lldb/include/lldb/Utility/DataExtractor.h
+++ lldb/include/lldb/Utility/DataExtractor.h
@@ -9,6 +9,7 @@
 #ifndef LLDB_UTILITY_DATAEXTRACTOR_H
 #define LLDB_UTILITY_DATAEXTRACTOR_H
 
+#include "lldb/Utility/Endian.h"
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-forward.h"
@@ -979,6 +980,27 @@
   }
 
 protected:
+  template  T Get(lldb::offset_t *offset_ptr, T fail_value) const {
+constexpr size_t src_size = sizeof(T);
+T val = fail_value;
+
+const T *src = static_cast(GetData(offset_ptr, src_size));
+if (!src)
+  return val;
+
+if (m_byte_order != endian::InlHostByteOrder()) {
+  const uint8_t *src_data = reinterpret_cast(src);
+  uint8_t *dst_data = reinterpret_cast(&val);
+  for (size_t i = 0; i < src_size; ++i)
+dst_data[src_size - 1 - i] = src_data[i];
+  return val;
+} else {
+  memcpy(&val, src, src_size);
+}
+
+return val;
+  }
+
   // Member variables
   const uint8_t *m_start; ///< A pointer to the first byte of data.
   const uint8_t


Index: lldb/source/Utility/DataExtractor.cpp
===
--- lldb/source/Utility/DataExtractor.cpp
+++ lldb/source/Utility/DataExtractor.cpp
@@ -15,7 +15,6 @@
 
 #include "lldb/Utility/DataBuffer.h"
 #include "lldb/Utility/DataBufferHeap.h"
-#include "lldb/Utility/Endian.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Stream.h"
@@ -624,41 +623,11 @@
 }
 
 float DataExtractor::GetFloat(offset_t *offset_ptr) const {
-  typedef float float_type;
-  float_type val = 0.0;
-  const size_t src_size = sizeof(float_type);
-  const float_type *src =
-  static_cast(GetData(offset_ptr, src_size));
-  if (src) {
-if (m_byte_order != endian::InlHostByteOrder()) {
-  const uint8_t *src_data = reinterpret_cast(src);
-  uint8_t *dst_data = reinterpret_cast(&val);
-  for (size_t i = 0; i < sizeof(float_type); ++i)
-dst_data[sizeof(float_type) - 1 - i] = src_data[i];
-} else {
-  val = *src;
-}
-  }
-  return val;
+  return Get(offset_ptr, 0.0);
 }
 
 double DataExtractor::GetDouble(offset_t *offset_ptr) const {
-  typedef double float_type;
-  float_type val = 0.0;
-  const size_t src_size = sizeof(float_type);
-  const float_type *src =
-  static_cast(GetData(offset_ptr, src_size));
-  if (src) {
-if (m_byte_order != endian::InlHostByteOrder()) {
-  const uint8_t *src_data = reinterpret_cast(src);
-  uint8_t *dst_data = reinterpret_cast(&val);
-  for (size_t i = 0; i < sizeof(float_type); ++i)
-dst_data[sizeof(float_type) - 1 - i] = src_data[i];
-} else {
-  val = *src;

[Lldb-commits] [PATCH] D83256: [lldb] Assert on unaligned load in DataExtractor

2020-07-06 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere marked an inline comment as done.
JDevlieghere added inline comments.



Comment at: lldb/include/lldb/Utility/DataExtractor.h:1000-1004
+  uint8_t *dst_data = reinterpret_cast(&val);
+  const uint8_t *src_data = reinterpret_cast(src);
+  for (size_t i = 0; i < src_size; ++i)
+dst_data[src_size - 1 - i] = src_data[i];
+  return val;

clayborg wrote:
> Might be nice to place this into a method function that swaps data of any 
> size. Then other functions can re-use it if needed.
I left it inline here so we can copy the data in place instead copy & swapping. 


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

https://reviews.llvm.org/D83256



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

[Lldb-commits] [PATCH] D83256: [lldb] Fix unaligned load in DataExtractor

2020-07-06 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg added a comment.

Just need some DataExtractorTest.cpp additions to test the unaligned loads for 
float and doubles?




Comment at: lldb/source/Utility/DataExtractor.cpp:626
 float DataExtractor::GetFloat(offset_t *offset_ptr) const {
-  typedef float float_type;
-  float_type val = 0.0;
-  const size_t src_size = sizeof(float_type);
-  const float_type *src =
-  static_cast(GetData(offset_ptr, src_size));
-  if (src) {
-if (m_byte_order != endian::InlHostByteOrder()) {
-  const uint8_t *src_data = reinterpret_cast(src);
-  uint8_t *dst_data = reinterpret_cast(&val);
-  for (size_t i = 0; i < sizeof(float_type); ++i)
-dst_data[sizeof(float_type) - 1 - i] = src_data[i];
-} else {
-  val = *src;
-}
-  }
-  return val;
+  return Get(offset_ptr, 0.0);
 }

nit: "0.0f" instead of "0.0"


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

https://reviews.llvm.org/D83256



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

[Lldb-commits] [PATCH] D83256: [lldb] Fix unaligned load in DataExtractor

2020-07-06 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg added a comment.

FYI: values like 4.0 or 2.25 work well for float values in tests as they encode 
perfectly on all systems.


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

https://reviews.llvm.org/D83256



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

[Lldb-commits] [PATCH] D83256: [lldb] Fix unaligned load in DataExtractor

2020-07-06 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg added a comment.

Here are the next values for 32 bit and 64 bit floats:

float a = 2.25; // 0x4010
double b = 2.25; // 0x4002

You might disable this on windows as I believe they have 10 byte floats for 
doubles?


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

https://reviews.llvm.org/D83256



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

[Lldb-commits] [PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

2020-07-06 Thread Shafik Yaghmour via Phabricator via lldb-commits 
shafik updated this revision to Diff 275865.
shafik added a comment.

Adding a second test that is not arm64 specific.


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

https://reviews.llvm.org/D83008

Files:
  clang/lib/AST/RecordLayoutBuilder.cpp
  lldb/test/Shell/Expr/Inputs/layout.cpp
  lldb/test/Shell/Expr/TestLayoutNonVirtualBaseClasses.test


Index: lldb/test/Shell/Expr/TestLayoutNonVirtualBaseClasses.test
===
--- /dev/null
+++ lldb/test/Shell/Expr/TestLayoutNonVirtualBaseClasses.test
@@ -0,0 +1,11 @@
+# RUN: %clangxx_host %p/Inputs/layout.cpp -g -o %t
+
+# RUN: %lldb %t -b -s %s | FileCheck %s
+
+expr (intptr_t)&d2g.ID - (intptr_t)&d2g
+# CHECK: (lldb) expr (intptr_t)&d2g.ID - (intptr_t)&d2g
+# CHECK: (long) $0 = 12
+
+expr (intptr_t)&d3g.f2 - (intptr_t)&d3g
+# CHECK: (lldb) expr (intptr_t)&d3g.f2 - (intptr_t)&d3g
+# CHECK: (long) $1 = 8
Index: lldb/test/Shell/Expr/Inputs/layout.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/Inputs/layout.cpp
@@ -0,0 +1,43 @@
+#include 
+
+struct B1 {
+  uint8_t a;
+};
+
+struct D1 : public B1 {
+  uint8_t a;
+  uint32_t ID;
+  uint8_t b;
+};
+
+struct Mixin : public D1 {
+  uint8_t a;
+  uint32_t *arr[3];
+};
+
+struct B2 {
+  uint32_t a;
+};
+
+class D2 : public B2, public Mixin {};
+
+struct B3 {
+  char f1;
+};
+
+struct alignas(8) B4 {
+  char f2;
+};
+
+struct D3 : B3, B4 {
+};
+
+D2 d2g;
+D3 d3g;
+
+int main() {
+  D2 d2;
+  D3 d3;
+
+  return d2.ID + d3.f2;
+}
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1187,11 +1187,10 @@
   // Query the external layout to see if it provides an offset.
   bool HasExternalLayout = false;
   if (UseExternalLayout) {
-// FIXME: This appears to be reversed.
 if (Base->IsVirtual)
-  HasExternalLayout = External.getExternalNVBaseOffset(Base->Class, 
Offset);
-else
   HasExternalLayout = External.getExternalVBaseOffset(Base->Class, Offset);
+else
+  HasExternalLayout = External.getExternalNVBaseOffset(Base->Class, 
Offset);
   }
 
   // Clang <= 6 incorrectly applied the 'packed' attribute to base classes.


Index: lldb/test/Shell/Expr/TestLayoutNonVirtualBaseClasses.test
===
--- /dev/null
+++ lldb/test/Shell/Expr/TestLayoutNonVirtualBaseClasses.test
@@ -0,0 +1,11 @@
+# RUN: %clangxx_host %p/Inputs/layout.cpp -g -o %t
+
+# RUN: %lldb %t -b -s %s | FileCheck %s
+
+expr (intptr_t)&d2g.ID - (intptr_t)&d2g
+# CHECK: (lldb) expr (intptr_t)&d2g.ID - (intptr_t)&d2g
+# CHECK: (long) $0 = 12
+
+expr (intptr_t)&d3g.f2 - (intptr_t)&d3g
+# CHECK: (lldb) expr (intptr_t)&d3g.f2 - (intptr_t)&d3g
+# CHECK: (long) $1 = 8
Index: lldb/test/Shell/Expr/Inputs/layout.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/Inputs/layout.cpp
@@ -0,0 +1,43 @@
+#include 
+
+struct B1 {
+  uint8_t a;
+};
+
+struct D1 : public B1 {
+  uint8_t a;
+  uint32_t ID;
+  uint8_t b;
+};
+
+struct Mixin : public D1 {
+  uint8_t a;
+  uint32_t *arr[3];
+};
+
+struct B2 {
+  uint32_t a;
+};
+
+class D2 : public B2, public Mixin {};
+
+struct B3 {
+  char f1;
+};
+
+struct alignas(8) B4 {
+  char f2;
+};
+
+struct D3 : B3, B4 {
+};
+
+D2 d2g;
+D3 d3g;
+
+int main() {
+  D2 d2;
+  D3 d3;
+
+  return d2.ID + d3.f2;
+}
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1187,11 +1187,10 @@
   // Query the external layout to see if it provides an offset.
   bool HasExternalLayout = false;
   if (UseExternalLayout) {
-// FIXME: This appears to be reversed.
 if (Base->IsVirtual)
-  HasExternalLayout = External.getExternalNVBaseOffset(Base->Class, Offset);
-else
   HasExternalLayout = External.getExternalVBaseOffset(Base->Class, Offset);
+else
+  HasExternalLayout = External.getExternalNVBaseOffset(Base->Class, Offset);
   }
 
   // Clang <= 6 incorrectly applied the 'packed' attribute to base classes.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

2020-07-06 Thread Shafik Yaghmour via Phabricator via lldb-commits 
shafik added a comment.

In D83008#2131776 , @teemperor wrote:

> Thanks for tracking this down, this is a really nasty bug...
>
> The fix itself is obviously fine, but I think I'm out of the loop regarding 
> the testing strategy. We talked about adding a Clang test for this with the 
> help of this layout overwrite JSON file. I assume that extending this to 
> cover virtual bases turned out to be more complicated than expected? FWIW, 
> I'm not necessarily the biggest fan of this Clang test option so I would be 
> fine if we leave it as-is.
>
> I think having an LLDB test is a good idea, but it's not clear why it's a 
> Shell test. If I understand correctly this test requires running on arm64 
> (so, a remote test target), but from what I remember all the remote platform 
> support is only in dotest.py? Also pretty much all other expression 
> evaluation tests and the utilities for that are in the Python/API test 
> infrastructure, so it's a bit out of place.
>
> Also I think the test can be in general much simpler than an arm64-specific 
> test. We get all base class offsets wrong in LLDB, so we can just write a 
> simple test where you change the layout of some structs in a way that it 
> doesn't fit the default layout. E.g., just putting `alignas` on a base class 
> and then reading fields should be enough to trigger the same bug.


Good idea using `alignas` that actually did the trick, I was having trouble 
getting this to reproduce otherwise. I added a second test which should also 
reproduce on non-arm64 cases but I will keep the original test as well since 
they are hitting the bug from slightly different paths.

The goal of using a shell test was to avoid writing a device specific test and 
even though the first case should also pass on all other platforms.


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

https://reviews.llvm.org/D83008



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

[Lldb-commits] [PATCH] D80105: [LLDB] Combine multiple defs of arm64 register sets

2020-07-06 Thread Muhammad Omair Javaid via Phabricator via lldb-commits 
omjavaid marked 10 inline comments as done.
omjavaid added inline comments.



Comment at: lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp:167
 assert(target_arch.GetTriple().getOS() == llvm::Triple::FreeBSD);
 switch (target_arch.GetMachine()) {
 case llvm::Triple::aarch64:

labath wrote:
> It would be good to merge these two switches. Then the reg(set)_interface 
> variables would be local to each case label and it would not be so weird that 
> we sometimes use one and sometimes the other.
I have tried a couple of options but the no of different combinations involved 
I feel current implementation should stay untill we incrementally move 
remaining architectures to user RegisterInfosAndSet interface.



Comment at: 
lldb/source/Plugins/Process/FreeBSD/RegisterContextPOSIXProcessMonitor_arm64.h:21
   lldb_private::Thread &thread, uint32_t concrete_frame_idx,
-  lldb_private::RegisterInfoInterface *register_info);
+  lldb_private::RegisterInfoAndSetInterface *register_info);
 

labath wrote:
> While we're changing interfaces, let's also make this 
> unique_ptr to document the ownership transfer. 
> (I might also drop the concrete_frame_idx argument, as that is always zero.)
Agreed. I will update this in updated revision.



Comment at: 
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:71-72
+
+  m_regset_interface_up = 
std::static_pointer_cast(
+  m_register_info_interface_up);
 }

labath wrote:
> The way I'd recommend dealing with this is to have a 
> `RegisterInfoPOSIX_arm64& GetRegisterInfo() const` method which performs a 
> cast on the `m_register_info_interface_up` pointer, and then have everything 
> call that. If you place that method in close proximity to this constructor, 
> then it should be clear that this operation is always safe. There's already 
> something similar being done in e.g. `NativeThreadLinux::GetProcess`.
yes this would be a lot cleaner than what I am doing currently. I am going to 
update this in next revision.



Comment at: lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp:82
 
 switch (arch.GetTriple().getOS()) {
 case llvm::Triple::FreeBSD: {

labath wrote:
> The reg(set)_interface and register_context switches could be merged here too 
> in a similar way...
Here also incremental merger will be a better approach IMO.


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

https://reviews.llvm.org/D80105



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

[Lldb-commits] [PATCH] D80105: [LLDB] Combine multiple defs of arm64 register sets

2020-07-06 Thread Muhammad Omair Javaid via Phabricator via lldb-commits 
omjavaid updated this revision to Diff 275869.
omjavaid added a comment.

This new revision incorporates all suggestions on previous version.

@labath What do you think? I have skipped merging switch statements and have 
picked all other suggestions you highlighted. If this is LGTM then i can rebase 
SVE patches on top of this?


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

https://reviews.llvm.org/D80105

Files:
  lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp
  
lldb/source/Plugins/Process/FreeBSD/RegisterContextPOSIXProcessMonitor_arm64.cpp
  lldb/source/Plugins/Process/FreeBSD/RegisterContextPOSIXProcessMonitor_arm64.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
  lldb/source/Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfoAndSetInterface.h
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
  lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp

Index: lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
===
--- lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
+++ lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
@@ -82,7 +82,6 @@
 case llvm::Triple::FreeBSD: {
   switch (arch.GetMachine()) {
   case llvm::Triple::aarch64:
-reg_interface = new RegisterInfoPOSIX_arm64(arch);
 break;
   case llvm::Triple::arm:
 reg_interface = new RegisterInfoPOSIX_arm(arch);
@@ -111,7 +110,6 @@
 case llvm::Triple::NetBSD: {
   switch (arch.GetMachine()) {
   case llvm::Triple::aarch64:
-reg_interface = new RegisterInfoPOSIX_arm64(arch);
 break;
   case llvm::Triple::x86_64:
 reg_interface = new RegisterContextNetBSD_x86_64(arch);
@@ -128,7 +126,6 @@
 reg_interface = new RegisterInfoPOSIX_arm(arch);
 break;
   case llvm::Triple::aarch64:
-reg_interface = new RegisterInfoPOSIX_arm64(arch);
 break;
   case llvm::Triple::mipsel:
   case llvm::Triple::mips:
@@ -159,7 +156,6 @@
 case llvm::Triple::OpenBSD: {
   switch (arch.GetMachine()) {
   case llvm::Triple::aarch64:
-reg_interface = new RegisterInfoPOSIX_arm64(arch);
 break;
   case llvm::Triple::arm:
 reg_interface = new RegisterInfoPOSIX_arm(arch);
@@ -180,7 +176,7 @@
   break;
 }
 
-if (!reg_interface) {
+if (!reg_interface && arch.GetMachine() != llvm::Triple::aarch64) {
   LLDB_LOGF(log, "elf-core::%s:: Architecture(%d) or OS(%d) not supported",
 __FUNCTION__, arch.GetMachine(), arch.GetTriple().getOS());
   assert(false && "Architecture or OS not supported");
@@ -189,7 +185,8 @@
 switch (arch.GetMachine()) {
 case llvm::Triple::aarch64:
   m_thread_reg_ctx_sp = std::make_shared(
-  *this, reg_interface, m_gpregset_data, m_notes);
+  *this, std::make_unique(arch),
+  m_gpregset_data, m_notes);
   break;
 case llvm::Triple::arm:
   m_thread_reg_ctx_sp = std::make_shared(
Index: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
===
--- lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
+++ lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
@@ -18,7 +18,7 @@
 public:
   RegisterContextCorePOSIX_arm64(
   lldb_private::Thread &thread,
-  lldb_private::RegisterInfoInterface *register_info,
+  std::unique_ptr register_info,
   const lldb_private::DataExtractor &gpregset,
   llvm::ArrayRef notes);
 
Index: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
===
--- lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
+++ lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
@@ -17,16 +17,16 @@
 using namespace lldb_private;
 
 RegisterContextCorePOSIX_arm64::RegisterContextCorePOSIX_arm64(
-Thread &thread, RegisterInfoInterface *register_info,
+Thread &thread, std::unique_ptr register_info,
 const DataExtractor &gpregset, llvm::ArrayRef notes)
-: RegisterContextPOSIX_arm64(thread, 0, register_info) {
+: RegisterContextPOSIX_arm64(thread, std::move(register_info)) {
   m_gpr_buffer = std::make_shared(gpregset.GetDataStart(),

[Lldb-commits] [PATCH] D83256: [lldb] Fix unaligned load in DataExtractor

2020-07-06 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere updated this revision to Diff 275886.
JDevlieghere added a comment.

Add unit tests


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

https://reviews.llvm.org/D83256

Files:
  lldb/include/lldb/Utility/DataExtractor.h
  lldb/source/Utility/DataExtractor.cpp
  lldb/unittests/Utility/DataExtractorTest.cpp

Index: lldb/unittests/Utility/DataExtractorTest.cpp
===
--- lldb/unittests/Utility/DataExtractorTest.cpp
+++ lldb/unittests/Utility/DataExtractorTest.cpp
@@ -299,3 +299,99 @@
   EXPECT_EQ(expected, BE.GetULEB128(&offset));
   EXPECT_EQ(9U, offset);
 }
+
+TEST(DataExtractorTest, GetFloat) {
+  float expected = 4.0f;
+  lldb::offset_t offset;
+
+  {
+uint8_t buffer[] = {0x00, 0x00, 0x80, 0x40};
+DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
+ sizeof(void *));
+
+offset = 0;
+EXPECT_DOUBLE_EQ(expected, LE.GetFloat(&offset));
+EXPECT_EQ(4U, offset);
+  }
+
+  {
+uint8_t buffer[] = {0x40, 0x80, 0x00, 0x00};
+DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig,
+ sizeof(void *));
+offset = 0;
+EXPECT_DOUBLE_EQ(expected, BE.GetFloat(&offset));
+EXPECT_EQ(4U, offset);
+  }
+}
+
+TEST(DataExtractorTest, GetFloatUnaligned) {
+  float expected = 4.0f;
+  lldb::offset_t offset;
+
+  {
+uint8_t buffer[] = {0x00, 0x00, 0x00, 0x80, 0x40};
+DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
+ sizeof(void *));
+
+offset = 1;
+EXPECT_DOUBLE_EQ(expected, LE.GetFloat(&offset));
+EXPECT_EQ(5U, offset);
+  }
+
+  {
+uint8_t buffer[] = {0x00, 0x40, 0x80, 0x00, 0x00};
+DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig,
+ sizeof(void *));
+offset = 1;
+EXPECT_DOUBLE_EQ(expected, BE.GetFloat(&offset));
+EXPECT_EQ(5U, offset);
+  }
+}
+
+TEST(DataExtractorTest, GetDouble) {
+  double expected = 4.0f;
+  lldb::offset_t offset;
+
+  {
+uint8_t buffer[] = {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x40};
+DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
+ sizeof(void *));
+
+offset = 0;
+EXPECT_DOUBLE_EQ(expected, LE.GetDouble(&offset));
+EXPECT_EQ(8U, offset);
+  }
+
+  {
+uint8_t buffer[] = {0x40, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
+DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig,
+ sizeof(void *));
+offset = 0;
+EXPECT_DOUBLE_EQ(expected, BE.GetDouble(&offset));
+EXPECT_EQ(8U, offset);
+  }
+}
+
+TEST(DataExtractorTest, GetDoubleUnaligned) {
+  float expected = 4.0f;
+  lldb::offset_t offset;
+
+  {
+uint8_t buffer[] = {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x40};
+DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
+ sizeof(void *));
+
+offset = 1;
+EXPECT_DOUBLE_EQ(expected, LE.GetDouble(&offset));
+EXPECT_EQ(9U, offset);
+  }
+
+  {
+uint8_t buffer[] = {0x00, 0x40, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
+DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig,
+ sizeof(void *));
+offset = 1;
+EXPECT_DOUBLE_EQ(expected, BE.GetDouble(&offset));
+EXPECT_EQ(9U, offset);
+  }
+}
Index: lldb/source/Utility/DataExtractor.cpp
===
--- lldb/source/Utility/DataExtractor.cpp
+++ lldb/source/Utility/DataExtractor.cpp
@@ -15,7 +15,6 @@
 
 #include "lldb/Utility/DataBuffer.h"
 #include "lldb/Utility/DataBufferHeap.h"
-#include "lldb/Utility/Endian.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Stream.h"
@@ -624,41 +623,11 @@
 }
 
 float DataExtractor::GetFloat(offset_t *offset_ptr) const {
-  typedef float float_type;
-  float_type val = 0.0;
-  const size_t src_size = sizeof(float_type);
-  const float_type *src =
-  static_cast(GetData(offset_ptr, src_size));
-  if (src) {
-if (m_byte_order != endian::InlHostByteOrder()) {
-  const uint8_t *src_data = reinterpret_cast(src);
-  uint8_t *dst_data = reinterpret_cast(&val);
-  for (size_t i = 0; i < sizeof(float_type); ++i)
-dst_data[sizeof(float_type) - 1 - i] = src_data[i];
-} else {
-  val = *src;
-}
-  }
-  return val;
+  return Get(offset_ptr, 0.0f);
 }
 
 double DataExtractor::GetDouble(offset_t *offset_ptr) const {
-  typedef double float_type;
-  float_type val = 0.0;
-  const size_t src_size = sizeof(float_type);
-  const float_type *src =
-  static_cast(GetData(offset_ptr, src_size));
-  if (src) {
-if (m_byte_order != endian::InlHostByteOrder()) {
-  const uint8_t *src_data = reinterpret_cast(src);
-  uint8_t *dst_data = reinterpret_cast(&val);
-  for (size_t i = 0; i < sizeof(float_type); ++i)
-dst_data[sizeof(float_type) - 1 - i] = src_data[i];
-} else {
-  val = *src;
-

[Lldb-commits] [PATCH] D83256: [lldb] Fix unaligned load in DataExtractor

2020-07-06 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Just need some sanity checks to make sure the sizeof(float) == 4 and 
sizeof(double) == 8 for the tests to make sure this doesn't fail on systems 
where that isn't true.




Comment at: lldb/unittests/Utility/DataExtractorTest.cpp:308
+  {
+uint8_t buffer[] = {0x00, 0x00, 0x80, 0x40};
+DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,

You might want to make check if the size of a float is 4 bytes here and return 
without testing anything if it isn't or this will fail.



Comment at: lldb/unittests/Utility/DataExtractorTest.cpp:332
+  {
+uint8_t buffer[] = {0x00, 0x00, 0x00, 0x80, 0x40};
+DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,

You might want to make check if the size of a float is 4 bytes here and return 
without testing anything if it isn't or this will fail.



Comment at: lldb/unittests/Utility/DataExtractorTest.cpp:356
+  {
+uint8_t buffer[] = {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x40};
+DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,

You might want to make check if the size of a double is 8 bytes here and return 
without testing anything if it isn't or this will fail. Windows used 10 byte 
floats for doubles I believe?



Comment at: lldb/unittests/Utility/DataExtractorTest.cpp:380
+  {
+uint8_t buffer[] = {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x40};
+DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,

You might want to make check if the size of a double is 8 bytes here and return 
without testing anything if it isn't or this will fail. Windows used 10 byte 
floats for doubles I believe?




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

https://reviews.llvm.org/D83256



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

[Lldb-commits] [PATCH] D83256: [lldb] Fix unaligned load in DataExtractor

2020-07-06 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere updated this revision to Diff 275890.
JDevlieghere added a comment.

I was stil checking the Windows float thing but skipping them based on the 
sizeof() seems more robust.


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

https://reviews.llvm.org/D83256

Files:
  lldb/include/lldb/Utility/DataExtractor.h
  lldb/source/Utility/DataExtractor.cpp
  lldb/unittests/Utility/DataExtractorTest.cpp

Index: lldb/unittests/Utility/DataExtractorTest.cpp
===
--- lldb/unittests/Utility/DataExtractorTest.cpp
+++ lldb/unittests/Utility/DataExtractorTest.cpp
@@ -299,3 +299,111 @@
   EXPECT_EQ(expected, BE.GetULEB128(&offset));
   EXPECT_EQ(9U, offset);
 }
+
+TEST(DataExtractorTest, GetFloat) {
+  if (sizeof(float) != 4)
+return;
+
+  float expected = 4.0f;
+  lldb::offset_t offset;
+
+  {
+uint8_t buffer[] = {0x00, 0x00, 0x80, 0x40};
+DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
+ sizeof(void *));
+
+offset = 0;
+EXPECT_DOUBLE_EQ(expected, LE.GetFloat(&offset));
+EXPECT_EQ(4U, offset);
+  }
+
+  {
+uint8_t buffer[] = {0x40, 0x80, 0x00, 0x00};
+DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig,
+ sizeof(void *));
+offset = 0;
+EXPECT_DOUBLE_EQ(expected, BE.GetFloat(&offset));
+EXPECT_EQ(4U, offset);
+  }
+}
+
+TEST(DataExtractorTest, GetFloatUnaligned) {
+  if (sizeof(float) != 4)
+return;
+
+  float expected = 4.0f;
+  lldb::offset_t offset;
+
+  {
+uint8_t buffer[] = {0x00, 0x00, 0x00, 0x80, 0x40};
+DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
+ sizeof(void *));
+
+offset = 1;
+EXPECT_DOUBLE_EQ(expected, LE.GetFloat(&offset));
+EXPECT_EQ(5U, offset);
+  }
+
+  {
+uint8_t buffer[] = {0x00, 0x40, 0x80, 0x00, 0x00};
+DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig,
+ sizeof(void *));
+offset = 1;
+EXPECT_DOUBLE_EQ(expected, BE.GetFloat(&offset));
+EXPECT_EQ(5U, offset);
+  }
+}
+
+TEST(DataExtractorTest, GetDouble) {
+  if (sizeof(float) != 8)
+return;
+
+  double expected = 4.0f;
+  lldb::offset_t offset;
+
+  {
+uint8_t buffer[] = {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x40};
+DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
+ sizeof(void *));
+
+offset = 0;
+EXPECT_DOUBLE_EQ(expected, LE.GetDouble(&offset));
+EXPECT_EQ(8U, offset);
+  }
+
+  {
+uint8_t buffer[] = {0x40, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
+DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig,
+ sizeof(void *));
+offset = 0;
+EXPECT_DOUBLE_EQ(expected, BE.GetDouble(&offset));
+EXPECT_EQ(8U, offset);
+  }
+}
+
+TEST(DataExtractorTest, GetDoubleUnaligned) {
+  if (sizeof(float) != 8)
+return;
+
+  float expected = 4.0f;
+  lldb::offset_t offset;
+
+  {
+uint8_t buffer[] = {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x40};
+DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
+ sizeof(void *));
+
+offset = 1;
+EXPECT_DOUBLE_EQ(expected, LE.GetDouble(&offset));
+EXPECT_EQ(9U, offset);
+  }
+
+  {
+uint8_t buffer[] = {0x00, 0x40, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
+DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig,
+ sizeof(void *));
+offset = 1;
+EXPECT_DOUBLE_EQ(expected, BE.GetDouble(&offset));
+EXPECT_EQ(9U, offset);
+  }
+}
Index: lldb/source/Utility/DataExtractor.cpp
===
--- lldb/source/Utility/DataExtractor.cpp
+++ lldb/source/Utility/DataExtractor.cpp
@@ -15,7 +15,6 @@
 
 #include "lldb/Utility/DataBuffer.h"
 #include "lldb/Utility/DataBufferHeap.h"
-#include "lldb/Utility/Endian.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Stream.h"
@@ -624,41 +623,11 @@
 }
 
 float DataExtractor::GetFloat(offset_t *offset_ptr) const {
-  typedef float float_type;
-  float_type val = 0.0;
-  const size_t src_size = sizeof(float_type);
-  const float_type *src =
-  static_cast(GetData(offset_ptr, src_size));
-  if (src) {
-if (m_byte_order != endian::InlHostByteOrder()) {
-  const uint8_t *src_data = reinterpret_cast(src);
-  uint8_t *dst_data = reinterpret_cast(&val);
-  for (size_t i = 0; i < sizeof(float_type); ++i)
-dst_data[sizeof(float_type) - 1 - i] = src_data[i];
-} else {
-  val = *src;
-}
-  }
-  return val;
+  return Get(offset_ptr, 0.0f);
 }
 
 double DataExtractor::GetDouble(offset_t *offset_ptr) const {
-  typedef double float_type;
-  float_type val = 0.0;
-  const size_t src_size = sizeof(float_type);
-  const float_type *src =
-  static_cast(GetData(offset_ptr, src_size));
-  if (src) {
-if (m_byte_order != endian::InlHostByteOrder()) {

[Lldb-commits] [PATCH] D83256: [lldb] Fix unaligned load in DataExtractor

2020-07-06 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

sizeof(double) instead of sizeof(float) for the double tests.




Comment at: lldb/unittests/Utility/DataExtractorTest.cpp:358
+TEST(DataExtractorTest, GetDouble) {
+  if (sizeof(float) != 8)
+return;

sizeof(double)



Comment at: lldb/unittests/Utility/DataExtractorTest.cpp:385
+TEST(DataExtractorTest, GetDoubleUnaligned) {
+  if (sizeof(float) != 8)
+return;

sizeof(double)


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

https://reviews.llvm.org/D83256



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

[Lldb-commits] [PATCH] D83256: [lldb] Fix unaligned load in DataExtractor

2020-07-06 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere updated this revision to Diff 275915.

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

https://reviews.llvm.org/D83256

Files:
  lldb/include/lldb/Utility/DataExtractor.h
  lldb/source/Utility/DataExtractor.cpp
  lldb/unittests/Utility/DataExtractorTest.cpp

Index: lldb/unittests/Utility/DataExtractorTest.cpp
===
--- lldb/unittests/Utility/DataExtractorTest.cpp
+++ lldb/unittests/Utility/DataExtractorTest.cpp
@@ -299,3 +299,111 @@
   EXPECT_EQ(expected, BE.GetULEB128(&offset));
   EXPECT_EQ(9U, offset);
 }
+
+TEST(DataExtractorTest, GetFloat) {
+  if (sizeof(float) != 4)
+return;
+
+  float expected = 4.0f;
+  lldb::offset_t offset;
+
+  {
+uint8_t buffer[] = {0x00, 0x00, 0x80, 0x40};
+DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
+ sizeof(void *));
+
+offset = 0;
+EXPECT_DOUBLE_EQ(expected, LE.GetFloat(&offset));
+EXPECT_EQ(4U, offset);
+  }
+
+  {
+uint8_t buffer[] = {0x40, 0x80, 0x00, 0x00};
+DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig,
+ sizeof(void *));
+offset = 0;
+EXPECT_DOUBLE_EQ(expected, BE.GetFloat(&offset));
+EXPECT_EQ(4U, offset);
+  }
+}
+
+TEST(DataExtractorTest, GetFloatUnaligned) {
+  if (sizeof(float) != 4)
+return;
+
+  float expected = 4.0f;
+  lldb::offset_t offset;
+
+  {
+uint8_t buffer[] = {0x00, 0x00, 0x00, 0x80, 0x40};
+DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
+ sizeof(void *));
+
+offset = 1;
+EXPECT_DOUBLE_EQ(expected, LE.GetFloat(&offset));
+EXPECT_EQ(5U, offset);
+  }
+
+  {
+uint8_t buffer[] = {0x00, 0x40, 0x80, 0x00, 0x00};
+DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig,
+ sizeof(void *));
+offset = 1;
+EXPECT_DOUBLE_EQ(expected, BE.GetFloat(&offset));
+EXPECT_EQ(5U, offset);
+  }
+}
+
+TEST(DataExtractorTest, GetDouble) {
+  if (sizeof(double) != 8)
+return;
+
+  double expected = 4.0f;
+  lldb::offset_t offset;
+
+  {
+uint8_t buffer[] = {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x40};
+DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
+ sizeof(void *));
+
+offset = 0;
+EXPECT_DOUBLE_EQ(expected, LE.GetDouble(&offset));
+EXPECT_EQ(8U, offset);
+  }
+
+  {
+uint8_t buffer[] = {0x40, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
+DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig,
+ sizeof(void *));
+offset = 0;
+EXPECT_DOUBLE_EQ(expected, BE.GetDouble(&offset));
+EXPECT_EQ(8U, offset);
+  }
+}
+
+TEST(DataExtractorTest, GetDoubleUnaligned) {
+  if (sizeof(double) != 8)
+return;
+
+  float expected = 4.0f;
+  lldb::offset_t offset;
+
+  {
+uint8_t buffer[] = {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x40};
+DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
+ sizeof(void *));
+
+offset = 1;
+EXPECT_DOUBLE_EQ(expected, LE.GetDouble(&offset));
+EXPECT_EQ(9U, offset);
+  }
+
+  {
+uint8_t buffer[] = {0x00, 0x40, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
+DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig,
+ sizeof(void *));
+offset = 1;
+EXPECT_DOUBLE_EQ(expected, BE.GetDouble(&offset));
+EXPECT_EQ(9U, offset);
+  }
+}
Index: lldb/source/Utility/DataExtractor.cpp
===
--- lldb/source/Utility/DataExtractor.cpp
+++ lldb/source/Utility/DataExtractor.cpp
@@ -15,7 +15,6 @@
 
 #include "lldb/Utility/DataBuffer.h"
 #include "lldb/Utility/DataBufferHeap.h"
-#include "lldb/Utility/Endian.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Stream.h"
@@ -624,41 +623,11 @@
 }
 
 float DataExtractor::GetFloat(offset_t *offset_ptr) const {
-  typedef float float_type;
-  float_type val = 0.0;
-  const size_t src_size = sizeof(float_type);
-  const float_type *src =
-  static_cast(GetData(offset_ptr, src_size));
-  if (src) {
-if (m_byte_order != endian::InlHostByteOrder()) {
-  const uint8_t *src_data = reinterpret_cast(src);
-  uint8_t *dst_data = reinterpret_cast(&val);
-  for (size_t i = 0; i < sizeof(float_type); ++i)
-dst_data[sizeof(float_type) - 1 - i] = src_data[i];
-} else {
-  val = *src;
-}
-  }
-  return val;
+  return Get(offset_ptr, 0.0f);
 }
 
 double DataExtractor::GetDouble(offset_t *offset_ptr) const {
-  typedef double float_type;
-  float_type val = 0.0;
-  const size_t src_size = sizeof(float_type);
-  const float_type *src =
-  static_cast(GetData(offset_ptr, src_size));
-  if (src) {
-if (m_byte_order != endian::InlHostByteOrder()) {
-  const uint8_t *src_data = reinterpret_cast(src);
-  uint8_t *dst_data = reinterpret_cast(&val);
-  for (size_t i = 0; i