[Lldb-commits] [lldb] r352845 - [PDB] Fix location retrieval for function local variables and arguments that are

[Aleksandr Urakov via lldb-commits]

Author: aleksandr.urakov
Date: Fri Feb  1 02:01:18 2019
New Revision: 352845

URL: http://llvm.org/viewvc/llvm-project?rev=352845&view=rev
Log:
[PDB] Fix location retrieval for function local variables and arguments that are
stored relative to VFRAME

Summary:
This patch makes LLDB able to retrieve proper values for function arguments and
local variables stored in PDB relative to VFRAME register.

Patch contains retrieval of corresponding FPO table entries from PDB and a
generic translator from FPO programs to DWARF expressions to get correct VFRAME
value.

Patch also improves variables-locations.test and makes this test passable on
x86.

Patch By: leonid.mashinsky

Reviewers: zturner, asmith, stella.stamenova, aleksandr.urakov

Reviewed By: zturner

Subscribers: arphaman, labath, mgorny, aprantl, JDevlieghere, lldb-commits

Tags: #lldb

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

Added:
lldb/trunk/source/Plugins/SymbolFile/NativePDB/CodeViewRegisterMapping.cpp
  - copied, changed from r352780, 
lldb/trunk/source/Plugins/SymbolFile/PDB/PDBLocationToDWARFExpression.cpp
lldb/trunk/source/Plugins/SymbolFile/NativePDB/CodeViewRegisterMapping.h

lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp

lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.h
lldb/trunk/unittests/SymbolFile/NativePDB/
lldb/trunk/unittests/SymbolFile/NativePDB/CMakeLists.txt

lldb/trunk/unittests/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpressionTests.cpp
Modified:
lldb/trunk/lit/SymbolFile/PDB/Inputs/VariablesLocationsTest.cpp
lldb/trunk/lit/SymbolFile/PDB/Inputs/VariablesLocationsTest.script
lldb/trunk/lit/SymbolFile/PDB/variables-locations.test
lldb/trunk/source/Expression/DWARFExpression.cpp
lldb/trunk/source/Plugins/SymbolFile/NativePDB/CMakeLists.txt
lldb/trunk/source/Plugins/SymbolFile/NativePDB/DWARFLocationExpression.cpp
lldb/trunk/source/Plugins/SymbolFile/NativePDB/DWARFLocationExpression.h
lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbIndex.h
lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp
lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbUtil.h
lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
lldb/trunk/source/Plugins/SymbolFile/PDB/CMakeLists.txt
lldb/trunk/source/Plugins/SymbolFile/PDB/PDBLocationToDWARFExpression.cpp
lldb/trunk/source/Plugins/SymbolFile/PDB/PDBLocationToDWARFExpression.h
lldb/trunk/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
lldb/trunk/unittests/SymbolFile/CMakeLists.txt

Modified: lldb/trunk/lit/SymbolFile/PDB/Inputs/VariablesLocationsTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/SymbolFile/PDB/Inputs/VariablesLocationsTest.cpp?rev=352845&r1=352844&r2=352845&view=diff
==
--- lldb/trunk/lit/SymbolFile/PDB/Inputs/VariablesLocationsTest.cpp (original)
+++ lldb/trunk/lit/SymbolFile/PDB/Inputs/VariablesLocationsTest.cpp Fri Feb  1 
02:01:18 2019
@@ -5,11 +5,22 @@ void __fastcall foo(short arg_0, float a
   double loc_1 = 0.5678;
 }
 
+__declspec(align(128)) struct S {
+  int a = 1234;
+};
+
+void bar(int arg_0) {
+ S loc_0;
+ int loc_1 = 5678;
+}
+
+
 int main(int argc, char *argv[]) {
   bool loc_0 = true;
   int loc_1 = ;
 
   foo(, 0.1234);
+  bar(22);
 
   return 0;
 }

Modified: lldb/trunk/lit/SymbolFile/PDB/Inputs/VariablesLocationsTest.script
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/SymbolFile/PDB/Inputs/VariablesLocationsTest.script?rev=352845&r1=352844&r2=352845&view=diff
==
--- lldb/trunk/lit/SymbolFile/PDB/Inputs/VariablesLocationsTest.script 
(original)
+++ lldb/trunk/lit/SymbolFile/PDB/Inputs/VariablesLocationsTest.script Fri Feb  
1 02:01:18 2019
@@ -1,4 +1,5 @@
 breakpoint set --file VariablesLocationsTest.cpp --line 6
+breakpoint set --file VariablesLocationsTest.cpp --line 15
 
 run
 
@@ -14,3 +15,11 @@ frame select 1
 
 frame variable loc_0
 frame variable loc_1
+
+continue
+
+frame variable arg_0
+
+frame variable loc_0
+frame variable loc_1
+

Modified: lldb/trunk/lit/SymbolFile/PDB/variables-locations.test
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/SymbolFile/PDB/variables-locations.test?rev=352845&r1=352844&r2=352845&view=diff
==
--- lldb/trunk/lit/SymbolFile/PDB/variables-locations.test (original)
+++ lldb/trunk/lit/SymbolFile/PDB/variables-locations.test Fri Feb  1 02:01:18 
2019
@@ -1,5 +1,6 @@
 REQUIRES: system-windows, lld
 RUN: %build --compiler=clang-cl --output=%t.exe 
%S/Inputs/VariablesLocationsTest.cpp
+RUN: env LLDB_USE_NATIVE_PDB_READER=0 %lldb -b -s 
%S/Inputs/VariablesLocationsTest.script -- %t.exe | FileCheck %s
 RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -b -s 
%S/Inputs/VariablesLocatio

[Lldb-commits] [PATCH] D55122: [PDB] Fix location retrieval for function local variables and arguments that are stored relative to VFRAME

[Aleksandr Urakov via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL352845: [PDB] Fix location retrieval for function local 
variables and arguments that are (authored by aleksandr.urakov, committed by ).
Herald added a project: LLVM.

Changed prior to commit:
  https://reviews.llvm.org/D55122?vs=184471&id=184695#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55122

Files:
  lldb/trunk/lit/SymbolFile/PDB/Inputs/VariablesLocationsTest.cpp
  lldb/trunk/lit/SymbolFile/PDB/Inputs/VariablesLocationsTest.script
  lldb/trunk/lit/SymbolFile/PDB/variables-locations.test
  lldb/trunk/source/Expression/DWARFExpression.cpp
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/CMakeLists.txt
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/CodeViewRegisterMapping.cpp
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/CodeViewRegisterMapping.h
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/DWARFLocationExpression.cpp
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/DWARFLocationExpression.h
  
lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
  
lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.h
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbIndex.h
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbUtil.h
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/trunk/source/Plugins/SymbolFile/PDB/CMakeLists.txt
  lldb/trunk/source/Plugins/SymbolFile/PDB/PDBLocationToDWARFExpression.cpp
  lldb/trunk/source/Plugins/SymbolFile/PDB/PDBLocationToDWARFExpression.h
  lldb/trunk/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
  lldb/trunk/unittests/SymbolFile/CMakeLists.txt
  lldb/trunk/unittests/SymbolFile/NativePDB/CMakeLists.txt
  
lldb/trunk/unittests/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpressionTests.cpp

Index: lldb/trunk/unittests/SymbolFile/CMakeLists.txt
===
--- lldb/trunk/unittests/SymbolFile/CMakeLists.txt
+++ lldb/trunk/unittests/SymbolFile/CMakeLists.txt
@@ -1,4 +1,5 @@
 add_subdirectory(DWARF)
+add_subdirectory(NativePDB)
 if (LLVM_ENABLE_DIA_SDK)
   add_subdirectory(PDB)
 endif()
Index: lldb/trunk/unittests/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpressionTests.cpp
===
--- lldb/trunk/unittests/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpressionTests.cpp
+++ lldb/trunk/unittests/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpressionTests.cpp
@@ -0,0 +1,166 @@
+//===-- PDBFPOProgramToDWARFExpressionTests.cpp -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.h"
+
+#include "lldb/Core/StreamBuffer.h"
+#include "lldb/Expression/DWARFExpression.h"
+#include "lldb/Utility/ArchSpec.h"
+#include "lldb/Utility/DataBufferHeap.h"
+#include "lldb/Utility/DataExtractor.h"
+#include "lldb/Utility/StreamString.h"
+
+using namespace lldb;
+using namespace lldb_private;
+using namespace lldb_private::npdb;
+
+/// Valid programs tests
+
+static void
+CheckValidProgramTranslation(llvm::StringRef fpo_program,
+ llvm::StringRef target_register_name,
+ llvm::StringRef expected_dwarf_expression) {
+  // initial setup
+  ArchSpec arch_spec("i686-pc-windows");
+  llvm::Triple::ArchType arch_type = arch_spec.GetMachine();
+  ByteOrder byte_order = arch_spec.GetByteOrder();
+  uint32_t address_size = arch_spec.GetAddressByteSize();
+  uint32_t byte_size = arch_spec.GetDataByteSize();
+
+  // program translation
+  StreamBuffer<32> stream(Stream::eBinary, address_size, byte_order);
+  ASSERT_TRUE(TranslateFPOProgramToDWARFExpression(
+  fpo_program, target_register_name, arch_type, stream));
+
+  // print dwarf expression to comparable textual representation
+  DataBufferSP buffer =
+  std::make_shared(stream.GetData(), stream.GetSize());
+  DataExtractor extractor(buffer, byte_order, address_size, byte_size);
+
+  StreamString result_dwarf_expression;
+  ASSERT_TRUE(DWARFExpression::PrintDWARFExpression(
+  result_dwarf_expression, extractor, address_size, 4, false));
+
+  // actual check
+  ASSERT_STREQ(expected_dwarf_expression.data(),
+   result_dwarf_expression.GetString().data());
+}
+
+TEST(PDBFPOProgramToDWARFExpressionTests, SingleAssignmentConst) {
+  CheckValidProgramTranslation("$T0 0 = ", "$T0", "DW_OP_constu 0x0");
+}
+
+TEST(PDBFPOProgramToDWARFExpressionTests, SingleAssignmentRegisterRef) {
+  CheckValidProgramTranslation("$T0 $ebp 

[Lldb-commits] [lldb] r352858 - [PDB] Fix build after r352845

[Aleksandr Urakov via lldb-commits]

Author: aleksandr.urakov
Date: Fri Feb  1 03:10:28 2019
New Revision: 352858

URL: http://llvm.org/viewvc/llvm-project?rev=352858&view=rev
Log:
[PDB] Fix build after r352845

Modified:
lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp

Modified: lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp?rev=352858&r1=352857&r2=352858&view=diff
==
--- lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp (original)
+++ lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp Fri Feb  1 
03:10:28 2019
@@ -508,7 +508,7 @@ VariableInfo lldb_private::npdb::GetVari
   return {};
 }
 
-static auto
+static llvm::FixedStreamArray::Iterator
 GetCorrespondingFrameData(lldb::addr_t load_addr,
   const DebugFrameDataSubsectionRef &fpo_data,
   const Variable::RangeList &ranges) {


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

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I have a lot of comments. The two major ones are:

- i think the way you link the tests is in the UB territory. I explain this in 
detail in one of the inline comments.
- I believe that your unit tests (not just in this patch) focus too much on 
testing the behavior of a single method, even when that method does not have an 
easily observable results. For this you then often need to expose internals of 
the tested class to be able to test some effect of that method on the internal 
state. This not all bad (i've done it myself sometimes), and  it's definitely 
better that not writing any unit tests. However, it's generally better to test 
the public interface of a method/class/entity, and in this case, I believe it 
should be possible. I'm imagining some tests like having a dummy class with a 
bunch of methods which are annotated with the SB_RECORD macros. Then, in the 
test you call the methods of this class with some arguments, have the repro 
framework serialize the calls to a (in-memory?) stream, and verify the contents 
of that stream. This will test a lot more code  -- the (De)Serialize functions 
are just fancy ways of invoking memcpy, but if you take all of that together 
with the SB_RECORD, SB_REGISTER macros, it becomes some pretty deep magic that 
is interesting to test exhaustively (it's fine, but not really interesting to 
test that Deserialize and Deserialize do the right thing for each of 
the possible fundamental types.) And if you use the deserializer interface to 
read out the recorded traces (instead of comparing raw bytes), then you can 
avoid depending on the endianness of the values.

Conversely, you can write a trace file with the serializer interface, and then 
tell the replayer to invoke the mock class which will verify that it was called 
with the right arguments.




Comment at: source/API/SBReproducer.cpp:1
+//===-- SBReproducer.cpp *- C++ 
-*-===//
+//

I found it weird to have one cpp file implementing two headers 
(`SBReproducer.h` and `SBReproducerPrivate.h`). Can you split it into two 
files? (This will come out naturally, if we split this up into modules as I 
mention in one of the other comments.)



Comment at: source/API/SBReproducer.cpp:69
+  repro::Loader *loader = repro::Reproducer::Instance().GetLoader();
+  if (!loader) {
+return false;

drop `{}` (that's what you seem to do elsewhere in this file).



Comment at: source/API/SBReproducerPrivate.h:44
+void *object = GetObjectForIndexImpl(idx);
+return static_cast::type *>(object);
+  }

Why do you need to `remove_const` here? If `T` is `const`, then const will be 
added back by the return statement anyway. If `T` is not `const`, then 
`remove_const` is a noop.



Comment at: source/API/SBReproducerPrivate.h:65-69
+auto it = m_mapping.find(idx);
+if (it == m_mapping.end()) {
+  return nullptr;
+}
+return m_mapping[idx];

replace by `m_mapping.lookup(idx)`, at which point you can consider dropping 
this function entirely.



Comment at: source/API/SBReproducerPrivate.h:82-93
+/// Base class for tag dispatch used in the SBDeserializer. Different tags are
+/// instantiated with different values.
+template  struct SBTag {};
+
+/// We need to differentiate between pointers to fundamental and
+/// non-fundamental types. See the corresponding SBDeserializer::Read method
+/// for the reason why.

Just curious: What's the advantage of this over just declaring a bunch of Tag 
classes directly (struct PointerTag{}; struct ReferenceTag{}; ...)?



Comment at: source/API/SBReproducerPrivate.h:121
+public:
+  SBDeserializer(llvm::StringRef buffer = {}) : m_buffer(buffer), m_offset(0) 
{}
+

Instead of the m_offset variable, what do you think about just `drop_front`ing 
the appropriate amount of bytes when you are done with the? It doesn't look 
like you'll ever need to go back to an earlier point in the stream...



Comment at: source/API/SBReproducerPrivate.h:155-161
+  // FIXME: We have references to this instance stored in replayer instance. We
+  // should find a better way to swap out the buffer after this instance has
+  // been created, but his will have to do for now.
+  void LoadBuffer(llvm::StringRef buffer) {
+m_buffer = buffer;
+m_offset = 0;
+  }

It sounds to me like this could be solved by passing the deserializer to the 
`operator()` of the SBReplayers instead of having it a member variable set by 
the constructor. This design also makes more sense to me, as theoretically 
there is no reason why all replay calls would have to come from the same 
deserializer object. You may even want to have a separate deserializer for each 
thread when you get around to replaying multithreaded recordings.

Af

[Lldb-commits] [PATCH] D57466: [lldb] Relax libc++ ABI version checking

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.

Yes, I suppose it is :)


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

https://reviews.llvm.org/D57466



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

[Lldb-commits] [PATCH] D57552: Handle "." in target.source-map in PathMapListing::FindFiles

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.
Herald added a subscriber: lldb-commits.



Comment at: 
packages/Python/lldbsuite/test/functionalities/source-map/TestTargetSourceMap.py:32-34
+#bp = target.BreakpointCreateByLocation(src_path, 2)
+#self.assertTrue(bp.GetNumLocations() == 1,
+#"make sure breakpoint was resolved with map")

Is this intentional?



Comment at: source/Target/PathMappingList.cpp:207
+
+if (orig_path.size() > 0) {
+  llvm::StringRef orig_ref(orig_path);

`!orig_path.empty()`?



Comment at: source/Target/PathMappingList.cpp:232
+  llvm::StringRef orig_remaining = orig_ref;
+  if (prefix_ref.size() == 0 
+  || orig_remaining.consume_front(prefix_ref)) {

The `size()` is actually unneeded as `consume_front` is trivially true for an 
empty StringRef.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D57552



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

[Lldb-commits] [PATCH] D57552: Handle "." in target.source-map in PathMapListing::FindFiles

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

FYI, @jingham , this is also an issue with DYLD plugins, if the file in the 
link map starts with ".".


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D57552



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

[Lldb-commits] [PATCH] D55332: [CMake] Python bindings generation polishing

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.
Herald added a project: LLDB.

@sgraenitz I've found an issue with this patch, using the Visual Studio 2015 
generator.

In scripts/CMakeLists.txt the old code (for Windows):

  if (CMAKE_CONFIGURATION_TYPES)
set(SWIG_PYTHON_DIR 
${CMAKE_BINARY_DIR}/\${CMAKE_INSTALL_CONFIG_NAME}/lib${LLVM_LIBDIR_SUFFIX}/site-packages)
  else()
set(SWIG_PYTHON_DIR 
${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX}/site-packages)
  endif()

Was changed to (for everything):

  set(SWIG_PYTHON_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR}/${swig_python_subdir})

Which changes /tools/lldb/scripts/cmake_install.cmake from

  if("${CMAKE_INSTALL_COMPONENT}" STREQUAL "Unspecified" OR NOT 
CMAKE_INSTALL_COMPONENT)
file(INSTALL DESTINATION "${CMAKE_INSTALL_PREFIX}/lib" TYPE DIRECTORY FILES 
"i:/obj/${CMAKE_INSTALL_CONFIG_NAME}/lib/site-packages")
  endif()

to

  if("${CMAKE_INSTALL_COMPONENT}" STREQUAL "Unspecified" OR NOT 
CMAKE_INSTALL_COMPONENT)
file(INSTALL DESTINATION "${CMAKE_INSTALL_PREFIX}/lib" TYPE DIRECTORY FILES 
"I:/obj/$(Configuration)/lib/site-packages")
  endif()

"$(Configuration)" is a Visual Studio variable, and not available to cmake, so 
our bots error out with this error:

  CMake Error at tools/lldb/scripts/cmake_install.cmake:31 (file):
file INSTALL cannot find "I:/obj/$(Configuration)/lib/site-packages".
  Call Stack (most recent call first):
tools/lldb/cmake_install.cmake:41 (include)
tools/cmake_install.cmake:41 (include)
cmake_install.cmake:45 (include)


Verified with the latest CMake (3.13.3).

Will switching back to the old code for Windows cause any issues?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55332



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

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D56322#1380442 , @labath wrote:

> I have a lot of comments. The two major ones are:
>
> - i think the way you link the tests is in the UB territory. I explain this 
> in detail in one of the inline comments.


Thanks, you're right and your suggestion makes sense. Keep in mind that the 
registry's `Init` method needs to have access to the SB definitions though.

> - I believe that your unit tests (not just in this patch) focus too much on 
> testing the behavior of a single method, even when that method does not have 
> an easily observable results. For this you then often need to expose 
> internals of the tested class to be able to test some effect of that method 
> on the internal state. This not all bad (i've done it myself sometimes), and  
> it's definitely better that not writing any unit tests.

It's funny you mention this, maybe I misremember but I recall you commenting on 
a patch that the current reproducers test were too high level. Maybe we have 
different views on what unit tests are, but I strongly believe that it's import 
to tests small *units* of code. They may seem trivial today until someone to 
decides to refactor them tomorrow. Indeed, we're already planning to move some 
of this code around and these tests will give me a lot more confidence. This 
stuff is relatively tricky, hard to debug and easy to get wrong. I strongly 
believe it's important to have these kind of unit tests, but I also totally 
agree that they should not be the only tests. See my next comment for more on 
that :-)

> However, it's generally better to test the public interface of a 
> method/class/entity, and in this case, I believe it should be possible.

It's 100% my fault for not making this clearer, but all that is in the 
pipeline. I wasn't able to work on this as much as I would've wanted the last 
week so that might have given the wrong impression. Maybe I should've kept the 
title as WIP, but I didn't want to discourage people from looking at the 
framework while I was adding this tests. Maybe I should've done this in a 
separate patch.

> I'm imagining some tests like having a dummy class with a bunch of methods 
> which are annotated with the SB_RECORD macros. Then, in the test you call the 
> methods of this class with some arguments, have the repro framework serialize 
> the calls to a (in-memory?) stream, and verify the contents of that stream. 
> This will test a lot more code  -- the (De)Serialize functions are just fancy 
> ways of invoking memcpy, but if you take all of that together with the 
> SB_RECORD, SB_REGISTER macros, it becomes some pretty deep magic that is 
> interesting to test exhaustively (it's fine, but not really interesting to 
> test that Deserialize and Deserialize do the right thing for each of 
> the possible fundamental types.) And if you use the deserializer interface to 
> read out the recorded traces (instead of comparing raw bytes), then you can 
> avoid depending on the endianness of the values.
> 
> Conversely, you can write a trace file with the serializer interface, and 
> then tell the replayer to invoke the mock class which will verify that it was 
> called with the right arguments.

This is very much what I'm planning to do. Coming back to my original point 
about testing smaller units, this is all great while it works, but it's going 
to be far from obvious as soon as this breaks. My hope is that if something 
fundamental breaks, one of the "obvious" unit test would catch it first.


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

https://reviews.llvm.org/D56322



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

[Lldb-commits] [PATCH] D57552: Handle "." in target.source-map in PathMapListing::FindFiles

[Jim Ingham via Phabricator via lldb-commits]

jingham updated this revision to Diff 184784.
jingham added a comment.

Fixed some inadvertently commented out test code (test still passes...)
Applied Pavel's suggestions (thanks!)


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D57552

Files:
  
packages/Python/lldbsuite/test/functionalities/source-map/TestTargetSourceMap.py
  packages/Python/lldbsuite/test/functionalities/source-map/a.yaml
  source/Target/PathMappingList.cpp

Index: source/Target/PathMappingList.cpp
===
--- source/Target/PathMappingList.cpp
+++ source/Target/PathMappingList.cpp
@@ -202,18 +202,36 @@
 bool PathMappingList::FindFile(const FileSpec &orig_spec,
FileSpec &new_spec) const {
   if (!m_pairs.empty()) {
-char orig_path[PATH_MAX];
-const size_t orig_path_len =
-orig_spec.GetPath(orig_path, sizeof(orig_path));
-if (orig_path_len > 0) {
+std::string orig_path = orig_spec.GetPath();
+
+if (!orig_path.empty()) {
+  llvm::StringRef orig_ref(orig_path);
+  bool orig_is_relative = orig_spec.IsRelative();
+  
   const_iterator pos, end = m_pairs.end();
   for (pos = m_pairs.begin(); pos != end; ++pos) {
-const size_t prefix_len = pos->first.GetLength();
+llvm::StringRef prefix_ref = pos->first.GetStringRef();
+if (orig_ref.size() >= prefix_ref.size()) {
+  // We consider a relative prefix or one of just "." to 
+  // mean "only apply to relative paths".
+  bool prefix_is_relative = false;
+  
+  if (prefix_ref == ".") {
+prefix_is_relative = true;
+// Remove the "." since it will have been removed from the
+// FileSpec paths already.
+prefix_ref = prefix_ref.drop_front();
+  } else {
+FileSpec prefix_spec(prefix_ref, FileSpec::Style::native);
+prefix_is_relative = prefix_spec.IsRelative();
+  }
+  if (prefix_is_relative != orig_is_relative)
+continue;
 
-if (orig_path_len >= prefix_len) {
-  if (::strncmp(pos->first.GetCString(), orig_path, prefix_len) == 0) {
+  llvm::StringRef orig_remaining = orig_ref;
+  if (orig_remaining.consume_front(prefix_ref)) {
 new_spec.SetFile(pos->second.GetCString(), FileSpec::Style::native);
-new_spec.AppendPathComponent(orig_path + prefix_len);
+new_spec.AppendPathComponent(orig_remaining);
 if (FileSystem::Instance().Exists(new_spec))
   return true;
   }
Index: packages/Python/lldbsuite/test/functionalities/source-map/a.yaml
===
--- packages/Python/lldbsuite/test/functionalities/source-map/a.yaml
+++ packages/Python/lldbsuite/test/functionalities/source-map/a.yaml
@@ -1,51 +1,31 @@
 --- !mach-o
-FileHeader:
+FileHeader:  
   magic:   0xFEEDFACF
   cputype: 0x0107
   cpusubtype:  0x0003
-  filetype:0x000A
-  ncmds:   6
-  sizeofcmds:  1376
-  flags:   0x
+  filetype:0x0001
+  ncmds:   4
+  sizeofcmds:  1160
+  flags:   0x2000
   reserved:0x
-LoadCommands:
-  - cmd: LC_UUID
-cmdsize: 24
-uuid:D37CC773-C218-3F97-99C9-CE4E77DDF2CE
-  - cmd: LC_SYMTAB
-cmdsize: 24
-symoff:  4096
-nsyms:   2
-stroff:  4128
-strsize: 28
+LoadCommands:
   - cmd: LC_SEGMENT_64
-cmdsize: 72
-segname: __PAGEZERO
+cmdsize: 1032
+segname: ''
 vmaddr:  0
-vmsize:  4294967296
-fileoff: 0
-filesize:0
-maxprot: 0
-initprot:0
-nsects:  0
-flags:   0
-  - cmd: LC_SEGMENT_64
-cmdsize: 232
-segname: __TEXT
-vmaddr:  4294967296
-vmsize:  4096
-fileoff: 0
-filesize:0
+vmsize:  744
+fileoff: 1192
+filesize:744
 maxprot: 7
-initprot:5
-nsects:  2
+initprot:7
+nsects:  12
 flags:   0
-Sections:
+Sections:
   - sectname:__text
 segname: __TEXT
-addr:0x00010FA0
-size:15
-offset:  0x
+addr:0x
+size:22
+offset:  0x04A8
 align:   4
 reloff:  0x
 nreloc:  0
@@ -53,203 +33,196 @@
 reserved1:   0x
 reserved2:   0x
 reserved3:   0x000

[Lldb-commits] [PATCH] D57552: Handle "." in target.source-map in PathMapListing::FindFiles

[Jim Ingham via Phabricator via lldb-commits]

jingham marked 5 inline comments as done.
jingham added inline comments.



Comment at: 
packages/Python/lldbsuite/test/functionalities/source-map/TestTargetSourceMap.py:32-34
+#bp = target.BreakpointCreateByLocation(src_path, 2)
+#self.assertTrue(bp.GetNumLocations() == 1,
+#"make sure breakpoint was resolved with map")

labath wrote:
> Is this intentional?
No.  The test still passes with these uncommented!



Comment at: source/Target/PathMappingList.cpp:207
+
+if (orig_path.size() > 0) {
+  llvm::StringRef orig_ref(orig_path);

labath wrote:
> `!orig_path.empty()`?
Doh!


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D57552



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

[Lldb-commits] [PATCH] D57552: Handle "." in target.source-map in PathMapListing::FindFiles

[Jim Ingham via Phabricator via lldb-commits]

jingham marked 2 inline comments as done.
jingham added a comment.

In D57552#1380782 , @ted wrote:

> FYI, @jingham , this is also an issue with DYLD plugins, if the file in the 
> link map starts with ".". "image search-paths add . /path/to/my/libraries" 
> fails in this case, because "./library.so" gets changed to "library.so".


This should fix that issue as well, since image search-paths are also 
PathMappingLists.  I don't see any tests for this, however.  Do you have 
something handy you could turn into a test?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D57552



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

[Lldb-commits] [PATCH] D57552: Handle "." in target.source-map in PathMapListing::FindFiles

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.



Comment at: source/Target/PathMappingList.cpp:204
FileSpec &new_spec) const {
   if (!m_pairs.empty()) {
+std::string orig_path = orig_spec.GetPath();

Can we put some early returns in here?

```
new_spec.Clear();

if (m_pairs.empty())
  return;
```



Comment at: source/Target/PathMappingList.cpp:207
+
+if (!orig_path.empty()) {
+  llvm::StringRef orig_ref(orig_path);

```
if (orig_path.empty())
  return;
```



Comment at: source/Target/PathMappingList.cpp:208
+if (!orig_path.empty()) {
+  llvm::StringRef orig_ref(orig_path);
+  bool orig_is_relative = orig_spec.IsRelative();

I think we can move this declaration inside of the for loop



Comment at: source/Target/PathMappingList.cpp:212
   const_iterator pos, end = m_pairs.end();
   for (pos = m_pairs.begin(); pos != end; ++pos) {
+llvm::StringRef prefix_ref = pos->first.GetStringRef();

How about 

```
for (const auto &pair : m_pairs) {
}
```



Comment at: source/Target/PathMappingList.cpp:232
+  llvm::StringRef orig_remaining = orig_ref;
+  if (orig_remaining.consume_front(prefix_ref)) {
 new_spec.SetFile(pos->second.GetCString(), 
FileSpec::Style::native);

After moving inside of the for loop, you can just write `if 
(orig_ref.consume_front(prefix_ref))`


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D57552



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

[Lldb-commits] [lldb] r352894 - Fix the xcode build for r352845.

[Jim Ingham via lldb-commits]

Author: jingham
Date: Fri Feb  1 10:26:08 2019
New Revision: 352894

URL: http://llvm.org/viewvc/llvm-project?rev=352894&view=rev
Log:
Fix the xcode build for r352845.

Modified:
lldb/trunk/lldb.xcodeproj/project.pbxproj

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

[Lldb-commits] [PATCH] D57552: Handle "." in target.source-map in PathMapListing::FindFiles

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

In D57552#1381046 , @jingham wrote:

> In D57552#1380782 , @ted wrote:
>
> > FYI, @jingham , this is also an issue with DYLD plugins, if the file in the 
> > link map starts with ".". "image search-paths add . /path/to/my/libraries" 
> > fails in this case, because "./library.so" gets changed to "library.so".
>
>
> This should fix that issue as well, since image search-paths are also 
> PathMappingLists.  I don't see any tests for this, however.  Do you have 
> something handy you could turn into a test?


I don't think this will fix it, because the problem happens in the initial 
FileSpec, before we get to search-paths.

From source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp, 
DYLDRendezvous::ReadSOEntryFromMemory():

  std::string file_path = ReadStringFromMemory(entry.path_addr);
  entry.file_spec.SetFile(file_path, false, FileSpec::Style::native);

FileSpec::SetFile normalizes the path, which will remove a leading "./", so 
"image search-paths add . /path/to/my/lib", which would work with "./mylib.so", 
won't work with "mylib.so".

I think I can write a test that uses LD_LIBRARY_PATH and a subdirectory so the 
target can load "./mylib.so", but LLDB won't be able to see it without a 
search-path.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D57552



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

[Lldb-commits] [PATCH] D57552: Handle "." in target.source-map in PathMapListing::FindFiles

[Jim Ingham via Phabricator via lldb-commits]

jingham updated this revision to Diff 184790.
jingham added a comment.

Implemented Zachary's suggestions.
Also added the source file sub-directory so the test will pass on other 
people's machines...


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

https://reviews.llvm.org/D57552

Files:
  
packages/Python/lldbsuite/test/functionalities/source-map/TestTargetSourceMap.py
  packages/Python/lldbsuite/test/functionalities/source-map/Trivial/main.c
  packages/Python/lldbsuite/test/functionalities/source-map/a.yaml
  source/Target/PathMappingList.cpp

Index: source/Target/PathMappingList.cpp
===
--- source/Target/PathMappingList.cpp
+++ source/Target/PathMappingList.cpp
@@ -201,26 +201,46 @@
 
 bool PathMappingList::FindFile(const FileSpec &orig_spec,
FileSpec &new_spec) const {
-  if (!m_pairs.empty()) {
-char orig_path[PATH_MAX];
-const size_t orig_path_len =
-orig_spec.GetPath(orig_path, sizeof(orig_path));
-if (orig_path_len > 0) {
-  const_iterator pos, end = m_pairs.end();
-  for (pos = m_pairs.begin(); pos != end; ++pos) {
-const size_t prefix_len = pos->first.GetLength();
-
-if (orig_path_len >= prefix_len) {
-  if (::strncmp(pos->first.GetCString(), orig_path, prefix_len) == 0) {
-new_spec.SetFile(pos->second.GetCString(), FileSpec::Style::native);
-new_spec.AppendPathComponent(orig_path + prefix_len);
-if (FileSystem::Instance().Exists(new_spec))
-  return true;
-  }
-}
+  if (m_pairs.empty())
+return false;
+  
+  std::string orig_path = orig_spec.GetPath();
+
+  if (orig_path.empty())
+return false;
+  
+  bool orig_is_relative = orig_spec.IsRelative();
+
+  const_iterator pos, end = m_pairs.end();
+  for (pos = m_pairs.begin(); pos != end; ++pos) {
+llvm::StringRef orig_ref(orig_path);
+llvm::StringRef prefix_ref = pos->first.GetStringRef();
+if (orig_ref.size() >= prefix_ref.size()) {
+  // We consider a relative prefix or one of just "." to
+  // mean "only apply to relative paths".
+  bool prefix_is_relative = false;
+  
+  if (prefix_ref == ".") {
+prefix_is_relative = true;
+// Remove the "." since it will have been removed from the
+// FileSpec paths already.
+prefix_ref = prefix_ref.drop_front();
+  } else {
+FileSpec prefix_spec(prefix_ref, FileSpec::Style::native);
+prefix_is_relative = prefix_spec.IsRelative();
+  }
+  if (prefix_is_relative != orig_is_relative)
+continue;
+
+  if (orig_ref.consume_front(prefix_ref)) {
+new_spec.SetFile(pos->second.GetCString(), FileSpec::Style::native);
+new_spec.AppendPathComponent(orig_ref);
+if (FileSystem::Instance().Exists(new_spec))
+  return true;
   }
 }
   }
+  
   new_spec.Clear();
   return false;
 }
Index: packages/Python/lldbsuite/test/functionalities/source-map/a.yaml
===
--- packages/Python/lldbsuite/test/functionalities/source-map/a.yaml
+++ packages/Python/lldbsuite/test/functionalities/source-map/a.yaml
@@ -1,51 +1,31 @@
 --- !mach-o
-FileHeader:
+FileHeader:  
   magic:   0xFEEDFACF
   cputype: 0x0107
   cpusubtype:  0x0003
-  filetype:0x000A
-  ncmds:   6
-  sizeofcmds:  1376
-  flags:   0x
+  filetype:0x0001
+  ncmds:   4
+  sizeofcmds:  1160
+  flags:   0x2000
   reserved:0x
-LoadCommands:
-  - cmd: LC_UUID
-cmdsize: 24
-uuid:D37CC773-C218-3F97-99C9-CE4E77DDF2CE
-  - cmd: LC_SYMTAB
-cmdsize: 24
-symoff:  4096
-nsyms:   2
-stroff:  4128
-strsize: 28
+LoadCommands:
   - cmd: LC_SEGMENT_64
-cmdsize: 72
-segname: __PAGEZERO
+cmdsize: 1032
+segname: ''
 vmaddr:  0
-vmsize:  4294967296
-fileoff: 0
-filesize:0
-maxprot: 0
-initprot:0
-nsects:  0
-flags:   0
-  - cmd: LC_SEGMENT_64
-cmdsize: 232
-segname: __TEXT
-vmaddr:  4294967296
-vmsize:  4096
-fileoff: 0
-filesize:0
+vmsize:  744
+fileoff: 1192
+filesize:744
 maxprot: 7
-initprot:5
-nsects:  2
+initprot:7
+nsects:  12
 flags:   0
-Sections:
+Sections:
   - sectname:__text
 segname: __TEXT
-addr:0x00010FA0
-size:15
-offset:  0x
+addr:

[Lldb-commits] [PATCH] D57552: Handle "." in target.source-map in PathMapListing::FindFiles

[Jim Ingham via Phabricator via lldb-commits]

jingham marked 5 inline comments as done.
jingham added a comment.

Sure, that's clearer.


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

https://reviews.llvm.org/D57552



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

[Lldb-commits] [PATCH] D57552: Handle "." in target.source-map in PathMapListing::FindFiles

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D57552#1381094 , @ted wrote:

> In D57552#1381046 , @jingham wrote:
>
> > In D57552#1380782 , @ted wrote:
> >
> > > FYI, @jingham , this is also an issue with DYLD plugins, if the file in 
> > > the link map starts with ".". "image search-paths add . 
> > > /path/to/my/libraries" fails in this case, because "./library.so" gets 
> > > changed to "library.so".
> >
> >
> > This should fix that issue as well, since image search-paths are also 
> > PathMappingLists.  I don't see any tests for this, however.  Do you have 
> > something handy you could turn into a test?
>
>
> I don't think this will fix it, because the problem happens in the initial 
> FileSpec, before we get to search-paths.
>
> From source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp, 
> DYLDRendezvous::ReadSOEntryFromMemory():
>
>   std::string file_path = ReadStringFromMemory(entry.path_addr);
>   entry.file_spec.SetFile(file_path, false, FileSpec::Style::native);
>   
>
> FileSpec::SetFile normalizes the path, which will remove a leading "./", so 
> "image search-paths add . /path/to/my/lib", which would work with 
> "./mylib.so", won't work with "mylib.so".
>
> I think I can write a test that uses LD_LIBRARY_PATH and a subdirectory so 
> the target can load "./mylib.so", but LLDB won't be able to see it without a 
> search-path.


That sounds like it will have to be fixed somewhere else, and not part of this 
patch then.  Can you file a bug to get that fixed?


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

https://reviews.llvm.org/D57552



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

[Lldb-commits] [PATCH] D57466: [lldb] Relax libc++ ABI version checking

[Tom Anderson via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL352899: [lldb] Relax libc++ ABI version checking (authored 
by thomasanderson, committed by ).
Herald added a project: LLVM.

Changed prior to commit:
  https://reviews.llvm.org/D57466?vs=184418&id=184798#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D57466

Files:
  lldb/trunk/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp

Index: lldb/trunk/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
===
--- lldb/trunk/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
+++ lldb/trunk/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
@@ -419,60 +419,49 @@
   .SetHideItemNames(false);
 
 #ifndef LLDB_DISABLE_PYTHON
-  lldb::TypeSummaryImplSP std_string_summary_sp(new CXXFunctionSummaryFormat(
-  stl_summary_flags,
-  lldb_private::formatters::LibcxxStringSummaryProviderASCII,
-  "std::string summary provider"));
-  lldb::TypeSummaryImplSP std_stringu16_summary_sp(new CXXFunctionSummaryFormat(
-  stl_summary_flags,
-  lldb_private::formatters::LibcxxStringSummaryProviderUTF16,
-  "std::u16string summary provider"));
-  lldb::TypeSummaryImplSP std_stringu32_summary_sp(new CXXFunctionSummaryFormat(
-  stl_summary_flags,
-  lldb_private::formatters::LibcxxStringSummaryProviderUTF32,
-  "std::u32string summary provider"));
-  lldb::TypeSummaryImplSP std_wstring_summary_sp(new CXXFunctionSummaryFormat(
-  stl_summary_flags, lldb_private::formatters::LibcxxWStringSummaryProvider,
-  "std::wstring summary provider"));
+  AddCXXSummary(cpp_category_sp,
+lldb_private::formatters::LibcxxStringSummaryProviderASCII,
+"std::string summary provider",
+ConstString("^std::__[[:alnum:]]+::string$"), stl_summary_flags,
+true);
+  AddCXXSummary(cpp_category_sp,
+lldb_private::formatters::LibcxxStringSummaryProviderASCII,
+"std::string summary provider",
+ConstString("^std::__[[:alnum:]]+::basic_string, "
+"std::__[[:alnum:]]+::allocator >$"),
+stl_summary_flags, true);
 
-  cpp_category_sp->GetTypeSummariesContainer()->Add(
-  ConstString("std::__1::string"), std_string_summary_sp);
-  cpp_category_sp->GetTypeSummariesContainer()->Add(
-  ConstString("std::__ndk1::string"), std_string_summary_sp);
-  cpp_category_sp->GetTypeSummariesContainer()->Add(
-  ConstString("std::__1::basic_string, "
-  "std::__1::allocator >"),
-  std_string_summary_sp);
-  cpp_category_sp->GetTypeSummariesContainer()->Add(
-  ConstString(
-  "std::__1::basic_string, "
-  "std::__1::allocator >"),
-  std_stringu16_summary_sp);
-  cpp_category_sp->GetTypeSummariesContainer()->Add(
-  ConstString(
-  "std::__1::basic_string, "
-  "std::__1::allocator >"),
-  std_stringu32_summary_sp);
-  cpp_category_sp->GetTypeSummariesContainer()->Add(
-  ConstString("std::__ndk1::basic_string, "
-  "std::__ndk1::allocator >"),
-  std_string_summary_sp);
+  AddCXXSummary(cpp_category_sp,
+lldb_private::formatters::LibcxxStringSummaryProviderUTF16,
+"std::u16string summary provider",
+ConstString(
+"^std::__[[:alnum:]]+::basic_string, "
+"std::__[[:alnum:]]+::allocator >$"),
+stl_summary_flags, true);
 
-  cpp_category_sp->GetTypeSummariesContainer()->Add(
-  ConstString("std::__1::wstring"), std_wstring_summary_sp);
-  cpp_category_sp->GetTypeSummariesContainer()->Add(
-  ConstString("std::__ndk1::wstring"), std_wstring_summary_sp);
-  cpp_category_sp->GetTypeSummariesContainer()->Add(
-  ConstString("std::__1::basic_string, "
-  "std::__1::allocator >"),
-  std_wstring_summary_sp);
-  cpp_category_sp->GetTypeSummariesContainer()->Add(
-  ConstString("std::__ndk1::basic_string, "
-  "std::__ndk1::allocator >"),
-  std_wstring_summary_sp);
+  AddCXXSummary(cpp_category_sp,
+lldb_private::formatters::LibcxxStringSummaryProviderUTF32,
+"std::u32string summary provider",
+ConstString(
+"^std::__[[:alnum:]]+::basic_string, "
+"std::__[[:alnum:]]+::allocator >$"),
+stl_summary_flags, true);
+
+  AddCXXSummary(cpp_category_sp,
+lldb_private::formatters::LibcxxWStringSummaryProvider,
+"std::wstring summary provider",
+ConstString("^std::__[[:alnum:]]+::wstring$"),
+stl_summary_flags, true);
+  AddCXXSummary(cpp_category_sp,
+lldb_private::formatters::LibcxxWStringSummaryProvider,
+  

[Lldb-commits] [lldb] r352901 - Revert "Fix the xcode build for r352845."

[Stefan Granitz via lldb-commits]

Author: stefan.graenitz
Date: Fri Feb  1 11:30:57 2019
New Revision: 352901

URL: http://llvm.org/viewvc/llvm-project?rev=352901&view=rev
Log:
Revert "Fix the xcode build for r352845."

This reverts commit 72c1213a5e901b80c0f1d2794e5088d7f71a3632.

Modified:
lldb/trunk/lldb.xcodeproj/project.pbxproj

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

[Lldb-commits] [lldb] r352902 - Fix the xcode build for r352845, attempt #2

[Stefan Granitz via lldb-commits]

Author: stefan.graenitz
Date: Fri Feb  1 11:31:02 2019
New Revision: 352902

URL: http://llvm.org/viewvc/llvm-project?rev=352902&view=rev
Log:
Fix the xcode build for r352845, attempt #2

Modified:
lldb/trunk/lldb.xcodeproj/project.pbxproj

Modified: lldb/trunk/lldb.xcodeproj/project.pbxproj
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lldb.xcodeproj/project.pbxproj?rev=352902&r1=352901&r2=352902&view=diff
==
--- lldb/trunk/lldb.xcodeproj/project.pbxproj (original)
+++ lldb/trunk/lldb.xcodeproj/project.pbxproj Fri Feb  1 11:31:02 2019
@@ -185,6 +185,8 @@
30B38A001CAAA6D7009524E3 /* ClangUtil.h in Headers */ = {isa = 
PBXBuildFile; fileRef = 3032B1B91CAAA44BE1AB /* ClangUtil.h */; };
2689006513353E0E00698AC0 /* ClangUtilityFunction.cpp in Sources 
*/ = {isa = PBXBuildFile; fileRef = 497C86BD122823D800B54702 /* 
ClangUtilityFunction.cpp */; };
949EEDA31BA76577008C63CF /* Cocoa.cpp in Sources */ = {isa = 
PBXBuildFile; fileRef = 949EEDA11BA76571008C63CF /* Cocoa.cpp */; };
+   5A6424962204D05000C3D9DB /* CodeViewRegisterMapping.cpp in 
Sources */ = {isa = PBXBuildFile; fileRef = 5A6424922204D04F00C3D9DB /* 
CodeViewRegisterMapping.cpp */; };
+   5A6424982204D05000C3D9DB /* CodeViewRegisterMapping.h in 
Headers */ = {isa = PBXBuildFile; fileRef = 5A6424942204D05000C3D9DB /* 
CodeViewRegisterMapping.h */; };
9441816E1C8F5EC900E5A8D9 /* CommandAlias.cpp in Sources */ = 
{isa = PBXBuildFile; fileRef = 9441816D1C8F5EC900E5A8D9 /* CommandAlias.cpp */; 
};
2689007F13353E2200698AC0 /* CommandCompletions.cpp in Sources 
*/ = {isa = PBXBuildFile; fileRef = 4C09CB74116BD98B00C7A725 /* 
CommandCompletions.cpp */; };
94BA8B70176F97CE005A91B5 /* CommandHistory.cpp in Sources */ = 
{isa = PBXBuildFile; fileRef = 94BA8B6F176F97CE005A91B5 /* CommandHistory.cpp 
*/; };
@@ -600,6 +602,8 @@
268900EE13353E6F00698AC0 /* PathMappingList.cpp in Sources */ = 
{isa = PBXBuildFile; fileRef = 495BBACB119A0DBE00418BEA /* PathMappingList.cpp 
*/; };
2668A2EE20AF417D00D94111 /* PathMappingListTest.cpp in Sources 
*/ = {isa = PBXBuildFile; fileRef = 2668A2ED20AF417D00D94111 /* 
PathMappingListTest.cpp */; };
AF815DF921C855B400023A34 /* PdbAstBuilder.cpp in Sources */ = 
{isa = PBXBuildFile; fileRef = AF815DF721C855B400023A34 /* PdbAstBuilder.cpp 
*/; };
+   5A6424952204D05000C3D9DB /* PdbFPOProgramToDWARFExpression.h in 
Headers */ = {isa = PBXBuildFile; fileRef = 5A6424912204D04F00C3D9DB /* 
PdbFPOProgramToDWARFExpression.h */; };
+   5A6424972204D05000C3D9DB /* PdbFPOProgramToDWARFExpression.cpp 
in Sources */ = {isa = PBXBuildFile; fileRef = 5A6424932204D04F00C3D9DB /* 
PdbFPOProgramToDWARFExpression.cpp */; };
AFD966BA217140B6006714AC /* PdbIndex.cpp in Sources */ = {isa = 
PBXBuildFile; fileRef = AFD966B6217140B6006714AC /* PdbIndex.cpp */; };
AF0F459E219FA1C800C1E612 /* PdbSymUid.cpp in Sources */ = {isa 
= PBXBuildFile; fileRef = AF0F459D219FA1C800C1E612 /* PdbSymUid.cpp */; };
AFD966B9217140B6006714AC /* PdbUtil.cpp in Sources */ = {isa = 
PBXBuildFile; fileRef = AFD966B5217140B6006714AC /* PdbUtil.cpp */; };
@@ -1616,6 +1620,8 @@
7F94D7172040A13A006EE3EA /* CleanUpTest.cpp */ = {isa = 
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; 
path = CleanUpTest.cpp; sourceTree = ""; };
949EEDA11BA76571008C63CF /* Cocoa.cpp */ = {isa = 
PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; name = Cocoa.cpp; 
path = Language/ObjC/Cocoa.cpp; sourceTree = ""; };
949EEDA21BA76571008C63CF /* Cocoa.h */ = {isa = 
PBXFileReference; lastKnownFileType = sourcecode.c.h; name = Cocoa.h; path = 
Language/ObjC/Cocoa.h; sourceTree = ""; };
+   5A6424942204D05000C3D9DB /* CodeViewRegisterMapping.h */ = {isa 
= PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name 
= CodeViewRegisterMapping.h; path = 
source/Plugins/SymbolFile/NativePDB/CodeViewRegisterMapping.h; sourceTree = 
SOURCE_ROOT; };
+   5A6424922204D04F00C3D9DB /* CodeViewRegisterMapping.cpp */ = 
{isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = 
sourcecode.cpp.cpp; name = CodeViewRegisterMapping.cpp; path = 
source/Plugins/SymbolFile/NativePDB/CodeViewRegisterMapping.cpp; sourceTree = 
SOURCE_ROOT; };
9441816D1C8F5EC900E5A8D9 /* CommandAlias.cpp */ = {isa = 
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; 
name = CommandAlias.cpp; path = source/Interpreter/CommandAlias.cpp; sourceTree 
= ""; };
9441816B1C8F5EB000E5A8D9 /* CommandAlias.h */ = {isa = 
PBXFileReference; lastKnownFileType = sourcecode.c.h; name = CommandAlias.h; 
path = include/lldb/Interpreter/CommandAlias.h; 

[Lldb-commits] [PATCH] D57552: Handle "." in target.source-map in PathMapListing::FindFiles

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

In D57552#1381126 , @jingham wrote:

> That sounds like it will have to be fixed somewhere else, and not part of 
> this patch then.  Can you file a bug to get that fixed?


Yeah, it's on my todo list. Which keeps growing!


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

https://reviews.llvm.org/D57552



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

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D56322#1380996 , @JDevlieghere 
wrote:

> In D56322#1380442 , @labath wrote:
>
> > I have a lot of comments. The two major ones are:
> >
> > - i think the way you link the tests is in the UB territory. I explain this 
> > in detail in one of the inline comments.
>
>
> Thanks, you're right and your suggestion makes sense. Keep in mind that the 
> registry's `Init` method needs to have access to the SB definitions though.


Yes, in this world, the I guess the version of the registry in the utility 
folder would be just an abstract class which provides the building blocks to 
intercept "any" api. And the API folder would contain an instantiation of this 
class which intercepts the SB methods. (And tests would 
instantiate/subclass/whatever it to intercept the mock methods.)

> 
> 
>> - I believe that your unit tests (not just in this patch) focus too much on 
>> testing the behavior of a single method, even when that method does not have 
>> an easily observable results. For this you then often need to expose 
>> internals of the tested class to be able to test some effect of that method 
>> on the internal state. This not all bad (i've done it myself sometimes), and 
>>  it's definitely better that not writing any unit tests.
> 
> It's funny you mention this, maybe I misremember but I recall you commenting 
> on a patch that the current reproducers test were too high level. Maybe we 
> have different views on what unit tests are, but I strongly believe that it's 
> import to tests small *units* of code. They may seem trivial today until 
> someone to decides to refactor them tomorrow. Indeed, we're already planning 
> to move some of this code around and these tests will give me a lot more 
> confidence. This stuff is relatively tricky, hard to debug and easy to get 
> wrong. I strongly believe it's important to have these kind of unit tests, 
> but I also totally agree that they should not be the only tests. See my next 
> comment for more on that :-)

Yes, that is funny, and a bit ironic. :) Though I think we're on the same page 
here. My comment was more about the lack of higher-level (or, shall we call 
them middle-level?) tests, then the presence of these low-level ones. Lldb has 
a very big deficiency in low-level tests, so I'd rather have more  (even if 
some end up being "change-detector tests") than less (and miss some 
opportunities to increase coverage) of them.

Bottom line: if you write some tests like the one I described previously, I am 
going to be _very_ happy. I have no problem with the existing tests staying, if 
you find them valuable. Just please make sure they work on big-endian platforms 
too. :)


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

https://reviews.llvm.org/D56322



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

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: source/API/SBReproducerPrivate.h:82-93
+/// Base class for tag dispatch used in the SBDeserializer. Different tags are
+/// instantiated with different values.
+template  struct SBTag {};
+
+/// We need to differentiate between pointers to fundamental and
+/// non-fundamental types. See the corresponding SBDeserializer::Read method
+/// for the reason why.

labath wrote:
> Just curious: What's the advantage of this over just declaring a bunch of Tag 
> classes directly (struct PointerTag{}; struct ReferenceTag{}; ...)?
Mostly preference, but the structs are less code so I've simplified it.



Comment at: source/API/SBReproducerPrivate.h:121
+public:
+  SBDeserializer(llvm::StringRef buffer = {}) : m_buffer(buffer), m_offset(0) 
{}
+

labath wrote:
> Instead of the m_offset variable, what do you think about just 
> `drop_front`ing the appropriate amount of bytes when you are done with the? 
> It doesn't look like you'll ever need to go back to an earlier point in the 
> stream...
We don't go back, but the buffer is the backing memory for deserialized 
c-strings.



Comment at: source/API/SBReproducerPrivate.h:182
+typedef typename std::remove_reference::type UnderlyingT;
+// If this is a reference to a fundamental type we just read its value.
+return *m_index_to_object.template GetObjectForIndex(

labath wrote:
> Is this correct? I would expect the fundamental references to not go through 
> this overload at all...
Pretty sure, but please elaborate on why you think that. 



Comment at: source/API/SBReproducerPrivate.h:453
+public:
+  SBSerializer(llvm::raw_ostream &stream = llvm::outs()) : m_stream(stream) {}
+

labath wrote:
> Is this default value used for anything? It don't see it being used, and it 
> doesn't seem particularly useful anyway, as you'd just get a lot of binary 
> junk on stdout.
I'm 100% sure it's needed because I got rid of it only to find out we couldn't 
do without it. I'm not entirely sure anymore, but I think we didn't know if the 
function was going to have a (useful) return value or not.



Comment at: tools/driver/Driver.cpp:913-917
+  SBReproducer reproducer;
+  if (reproducer.Replay()) {
+return 0;
+  }
+

labath wrote:
> The driver should know whether it is replaying things or not, right? Can we 
> have it call `Replay` only if replay mode has actually been requested?
Replay will check if we're in replay mode, which I think is a more canonical 
way to check it. Happy to change this if you disagree.


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

https://reviews.llvm.org/D56322



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

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 184825.
JDevlieghere marked 27 inline comments as done.
JDevlieghere added a comment.

Address Pavel's inline comments, modulo

- Fixing the ODR violation (should be easier to verify inline comments are 
addressed before moving the code).
- Fixing the endianness in the unit test  (I originally considered ifdef'ing 
this, but that combined with the float-encoding made me reconsider).


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

https://reviews.llvm.org/D56322

Files:
  include/lldb/API/SBReproducer.h
  source/API/CMakeLists.txt
  source/API/SBReproducer.cpp
  source/API/SBReproducerPrivate.h
  tools/driver/Driver.cpp
  unittests/API/CMakeLists.txt
  unittests/API/SBReproducerTest.cpp
  unittests/CMakeLists.txt

Index: unittests/CMakeLists.txt
===
--- unittests/CMakeLists.txt
+++ unittests/CMakeLists.txt
@@ -58,6 +58,7 @@
 endfunction()
 
 add_subdirectory(TestingSupport)
+add_subdirectory(API)
 add_subdirectory(Breakpoint)
 add_subdirectory(Core)
 add_subdirectory(Disassembler)
Index: unittests/API/SBReproducerTest.cpp
===
--- /dev/null
+++ unittests/API/SBReproducerTest.cpp
@@ -0,0 +1,232 @@
+//===-- SBReproducerTest.cpp *- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "SBReproducerPrivate.h"
+
+using namespace lldb_private;
+using namespace lldb_private::repro;
+
+namespace {
+struct Foo {};
+struct Bar {};
+
+struct Pod {
+  bool a = true;
+  bool b = false;
+  char c = 'a';
+  float d = 1.1;
+  int e = 2;
+  long long f = 3;
+  long g = 4;
+  short h = 5;
+  unsigned char i = 'b';
+  unsigned int j = 6;
+  unsigned long long k = 7;
+  unsigned long l = 8;
+  unsigned short m = 9;
+
+  unsigned char data[52] = {
+  0x01, 0x00, 0x61, 0xcd, 0xcc, 0x8c, 0x3f, 0x02, 0x00, 0x00, 0x00,
+  0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x04, 0x00, 0x00,
+  0x00, 0x00, 0x00, 0x00, 0x00, 0x05, 0x00, 0x62, 0x06, 0x00, 0x00,
+  0x00, 0x07, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x08, 0x00,
+  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x09, 0x00};
+};
+
+class TestingSBDeserializer : public SBDeserializer {
+public:
+  using SBDeserializer::GetSBIndexToObject;
+  using SBDeserializer::SBDeserializer;
+};
+} // namespace
+
+static const Pod p;
+
+TEST(SBIndexToObjectTest, ObjectForIndex) {
+  SBIndexToObject index_to_object;
+  Foo foo;
+  Bar bar;
+
+  EXPECT_EQ(nullptr, index_to_object.GetObjectForIndex(1));
+  EXPECT_EQ(nullptr, index_to_object.GetObjectForIndex(2));
+
+  index_to_object.AddObjectForIndex(1, foo);
+  index_to_object.AddObjectForIndex(2, &bar);
+
+  EXPECT_EQ(&foo, index_to_object.GetObjectForIndex(1));
+  EXPECT_EQ(&bar, index_to_object.GetObjectForIndex(2));
+}
+
+TEST(SBDeserializerTest, HasData) {
+  {
+SBDeserializer deserializer("");
+EXPECT_FALSE(deserializer.HasData(1));
+  }
+
+  {
+SBDeserializer deserializer("a");
+EXPECT_TRUE(deserializer.HasData(1));
+EXPECT_FALSE(deserializer.HasData(2));
+  }
+}
+
+TEST(SBDeserializerTest, DeserializePod) {
+  llvm::StringRef buffer(reinterpret_cast(p.data), 52);
+  SBDeserializer deserializer(buffer);
+
+  EXPECT_EQ(p.a, deserializer.Deserialize());
+  EXPECT_EQ(p.b, deserializer.Deserialize());
+  EXPECT_EQ(p.c, deserializer.Deserialize());
+  EXPECT_EQ(p.d, deserializer.Deserialize());
+  EXPECT_EQ(p.e, deserializer.Deserialize());
+  EXPECT_EQ(p.f, deserializer.Deserialize());
+  EXPECT_EQ(p.g, deserializer.Deserialize());
+  EXPECT_EQ(p.h, deserializer.Deserialize());
+  EXPECT_EQ(p.i, deserializer.Deserialize());
+  EXPECT_EQ(p.j, deserializer.Deserialize());
+  EXPECT_EQ(p.k, deserializer.Deserialize());
+  EXPECT_EQ(p.l, deserializer.Deserialize());
+  EXPECT_EQ(p.m, deserializer.Deserialize());
+}
+
+TEST(SBDeserializerTest, DeserializePodPointer) {
+  llvm::StringRef buffer(reinterpret_cast(p.data), 52);
+  SBDeserializer deserializer(buffer);
+
+  EXPECT_EQ(p.a, *deserializer.Deserialize());
+  EXPECT_EQ(p.b, *deserializer.Deserialize());
+  EXPECT_EQ(p.c, *deserializer.Deserialize());
+  EXPECT_EQ(p.d, *deserializer.Deserialize());
+  EXPECT_EQ(p.e, *deserializer.Deserialize());
+  EXPECT_EQ(p.f, *deserializer.Deserialize());
+  EXPECT_EQ(p.g, *deserializer.Deserialize());
+  EXPECT_EQ(p.h, *deserializer.Deserialize());
+  EXPECT_EQ(p.i, *deserializer.Deserialize());
+  EXPECT_EQ(p.j, *deserializer.Deserialize());
+  EXPECT_EQ(p.k, *deserializer.Deserialize());
+  EXPECT_EQ(p.l, *deserializer.Deserialize());
+  EXPECT_EQ(p.m, *deserializer.Deserialize());

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D56322#1381254 , @labath wrote:

> In D56322#1380996 , @JDevlieghere 
> wrote:
>
> > In D56322#1380442 , @labath wrote:
> >
> > > I have a lot of comments. The two major ones are:
> > >
> > > - i think the way you link the tests is in the UB territory. I explain 
> > > this in detail in one of the inline comments.
> >
> >
> > Thanks, you're right and your suggestion makes sense. Keep in mind that the 
> > registry's `Init` method needs to have access to the SB definitions though.
>
>
> Yes, in this world, the I guess the version of the registry in the utility 
> folder would be just an abstract class which provides the building blocks to 
> intercept "any" api. And the API folder would contain an instantiation of 
> this class which intercepts the SB methods. (And tests would 
> instantiate/subclass/whatever it to intercept the mock methods.)
>
> > 
> > 
> >> - I believe that your unit tests (not just in this patch) focus too much 
> >> on testing the behavior of a single method, even when that method does not 
> >> have an easily observable results. For this you then often need to expose 
> >> internals of the tested class to be able to test some effect of that 
> >> method on the internal state. This not all bad (i've done it myself 
> >> sometimes), and  it's definitely better that not writing any unit tests.
> > 
> > It's funny you mention this, maybe I misremember but I recall you 
> > commenting on a patch that the current reproducers test were too high 
> > level. Maybe we have different views on what unit tests are, but I strongly 
> > believe that it's import to tests small *units* of code. They may seem 
> > trivial today until someone to decides to refactor them tomorrow. Indeed, 
> > we're already planning to move some of this code around and these tests 
> > will give me a lot more confidence. This stuff is relatively tricky, hard 
> > to debug and easy to get wrong. I strongly believe it's important to have 
> > these kind of unit tests, but I also totally agree that they should not be 
> > the only tests. See my next comment for more on that :-)
>
> Yes, that is funny, and a bit ironic. :) Though I think we're on the same 
> page here. My comment was more about the lack of higher-level (or, shall we 
> call them middle-level?) tests, then the presence of these low-level ones. 
> Lldb has a very big deficiency in low-level tests, so I'd rather have more  
> (even if some end up being "change-detector tests") than less (and miss some 
> opportunities to increase coverage) of them.


Great :-)

> Bottom line: if you write some tests like the one I described previously, I 
> am going to be _very_ happy. I have no problem with the existing tests 
> staying, if you find them valuable. Just please make sure they work on 
> big-endian platforms too. :)

Awesome, yeah I didn't consider the float encoding, I'll just do a roundtrip 
test.


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

https://reviews.llvm.org/D56322



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

[Lldb-commits] [PATCH] D57552: Handle "." in target.source-map in PathMapListing::FindFiles

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: source/Target/PathMappingList.cpp:223
+  
+  if (prefix_ref == ".") {
+prefix_is_relative = true;

We are we finding a "." in any path now? I thought we normalized those all out? 
Can a PathMappingList contain non normalized paths? if so, can't we just 
normalize them as they go into the list?


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

https://reviews.llvm.org/D57552



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

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 184874.
JDevlieghere added a comment.

- Move framework in Utility
- Update tests


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

https://reviews.llvm.org/D56322

Files:
  include/lldb/API/SBReproducer.h
  include/lldb/Utility/ReproducerInstrumentation.h
  source/API/CMakeLists.txt
  source/API/SBReproducer.cpp
  source/API/SBReproducerPrivate.h
  source/Utility/CMakeLists.txt
  source/Utility/ReproducerInstrumentation.cpp
  tools/driver/Driver.cpp
  unittests/Utility/CMakeLists.txt
  unittests/Utility/ReproducerInstrumentationTest.cpp

Index: unittests/Utility/ReproducerInstrumentationTest.cpp
===
--- /dev/null
+++ unittests/Utility/ReproducerInstrumentationTest.cpp
@@ -0,0 +1,209 @@
+//===-- ReproducerTest.cpp *- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "lldb/Utility/ReproducerInstrumentation.h"
+
+using namespace lldb_private;
+using namespace lldb_private::repro;
+
+namespace {
+struct Foo {};
+struct Bar {};
+
+struct Pod {
+  bool a = true;
+  bool b = false;
+  char c = 'a';
+  float d = 1.1;
+  int e = 2;
+  long long f = 3;
+  long g = 4;
+  short h = 5;
+  unsigned char i = 'b';
+  unsigned int j = 6;
+  unsigned long long k = 7;
+  unsigned long l = 8;
+  unsigned short m = 9;
+};
+
+class TestingDeserializer : public Deserializer {
+public:
+  using Deserializer::Deserializer;
+  using Deserializer::GetIndexToObject;
+};
+} // namespace
+
+static const Pod p;
+
+TEST(IndexToObjectTest, ObjectForIndex) {
+  IndexToObject index_to_object;
+  Foo foo;
+  Bar bar;
+
+  EXPECT_EQ(nullptr, index_to_object.GetObjectForIndex(1));
+  EXPECT_EQ(nullptr, index_to_object.GetObjectForIndex(2));
+
+  index_to_object.AddObjectForIndex(1, foo);
+  index_to_object.AddObjectForIndex(2, &bar);
+
+  EXPECT_EQ(&foo, index_to_object.GetObjectForIndex(1));
+  EXPECT_EQ(&bar, index_to_object.GetObjectForIndex(2));
+}
+
+TEST(DeserializerTest, HasData) {
+  {
+Deserializer deserializer("");
+EXPECT_FALSE(deserializer.HasData(1));
+  }
+
+  {
+Deserializer deserializer("a");
+EXPECT_TRUE(deserializer.HasData(1));
+EXPECT_FALSE(deserializer.HasData(2));
+  }
+}
+
+TEST(SerializationRountripTest, SerializeDeserializePod) {
+  std::string str;
+  llvm::raw_string_ostream os(str);
+
+  Serializer serializer(os);
+  serializer.SerializeAll(p.a, p.b, p.c, p.d, p.e, p.f, p.g, p.h, p.i, p.j, p.k,
+  p.l, p.m);
+
+  llvm::StringRef buffer(os.str());
+  Deserializer deserializer(buffer);
+
+  EXPECT_EQ(p.a, deserializer.Deserialize());
+  EXPECT_EQ(p.b, deserializer.Deserialize());
+  EXPECT_EQ(p.c, deserializer.Deserialize());
+  EXPECT_EQ(p.d, deserializer.Deserialize());
+  EXPECT_EQ(p.e, deserializer.Deserialize());
+  EXPECT_EQ(p.f, deserializer.Deserialize());
+  EXPECT_EQ(p.g, deserializer.Deserialize());
+  EXPECT_EQ(p.h, deserializer.Deserialize());
+  EXPECT_EQ(p.i, deserializer.Deserialize());
+  EXPECT_EQ(p.j, deserializer.Deserialize());
+  EXPECT_EQ(p.k, deserializer.Deserialize());
+  EXPECT_EQ(p.l, deserializer.Deserialize());
+  EXPECT_EQ(p.m, deserializer.Deserialize());
+}
+
+TEST(SerializationRountripTest, SerializeDeserializePodPointers) {
+  std::string str;
+  llvm::raw_string_ostream os(str);
+
+  Serializer serializer(os);
+  serializer.SerializeAll(&p.a, &p.b, &p.c, &p.d, &p.e, &p.f, &p.g, &p.h, &p.i,
+  &p.j, &p.k, &p.l, &p.m);
+
+  llvm::StringRef buffer(os.str());
+  Deserializer deserializer(buffer);
+
+  EXPECT_EQ(p.a, *deserializer.Deserialize());
+  EXPECT_EQ(p.b, *deserializer.Deserialize());
+  EXPECT_EQ(p.c, *deserializer.Deserialize());
+  EXPECT_EQ(p.d, *deserializer.Deserialize());
+  EXPECT_EQ(p.e, *deserializer.Deserialize());
+  EXPECT_EQ(p.f, *deserializer.Deserialize());
+  EXPECT_EQ(p.g, *deserializer.Deserialize());
+  EXPECT_EQ(p.h, *deserializer.Deserialize());
+  EXPECT_EQ(p.i, *deserializer.Deserialize());
+  EXPECT_EQ(p.j, *deserializer.Deserialize());
+  EXPECT_EQ(p.k, *deserializer.Deserialize());
+  EXPECT_EQ(p.l, *deserializer.Deserialize());
+  EXPECT_EQ(p.m, *deserializer.Deserialize());
+}
+
+TEST(SerializationRountripTest, SerializeDeserializePodReferences) {
+  std::string str;
+  llvm::raw_string_ostream os(str);
+
+  Serializer serializer(os);
+  serializer.SerializeAll(p.a, p.b, p.c, p.d, p.e, p.f, p.g, p.h, p.i, p.j, p.k,
+  p.l, p.m);
+
+  llvm::StringRef buffer(os.str());
+  Deserializer deserializer(buffer);
+
+  EXPECT_EQ(p.a, deserializer.Deserialize());
+  EXPECT_EQ(p.b, deserializer.D