[Lldb-commits] [PATCH] D89812: [lldb][PDB] Add ObjectFile PDB plugin

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:176-197
+  switch (dbi_stream->getMachineType()) {
+  case PDB_Machine::Amd64:
+spec.SetTriple("x86_64-pc-windows");
+specs.Append(module_spec);
+break;
+  case PDB_Machine::x86:
+spec.SetTriple("i386-pc-windows");

zequanwu wrote:
> labath wrote:
> > Could this use the same algorithm as `ObjectFilePDB::GetArchitecture` ?
> No. This part is the same algorithm as the part in 
> `ObjectFilePECOFF::GetModuleSpecifications`, 
> https://github.com/llvm/llvm-project/blob/master/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp#L194-L215.
> 
> The `ObjectFilePDB::GetArchitecture` is same as 
> `ObjectFilePECOFF::GetArchitecture`.
I'm not sure this kind of consistency actually helps. For example, in 
`Module::MatchesModuleSpec`, module's own architecture (as provided by 
`ObjectFile(PECOFF)::GetArchitecture`) is compared to the one in the ModuleSpec 
(generally coming from `ObjectFile(PDB)::GetModuleSpecifications`.

If there's any mismatch between the two ways of computing a ArchSpec, then 
there will be trouble, even though they are "symmetric".

That said, I think we can keep this like it is in this patch, but I'd like to 
clean up both implementations in a another patch.



Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:207-209
+Address ObjectFilePDB::GetBaseAddress() {
+  return Address(GetSectionList()->GetSectionAtIndex(0), 0);
+}

zequanwu wrote:
> labath wrote:
> > Leave this unimplemented. PDBs don't have sections and are not loaded into 
> > memory, so the function does not apply.
> This is actually necessary. It's called at 
> `SymbolFileNativePDB::InitializeObject`. Otherwise, setting breakpoint won't 
> work.
So, that's a bug in `SymbolFileNativePDB::InitializeObject`. It should be 
calling `m_objfile_sp->GetModule()->GetObjectFile()->GetBaseAddress()` (as done 
in e.g. `SymbolFileBreakpad::GetBaseFileAddress`) instead of 
`m_objfile_sp->GetBaseAddress()`. That will get you the base address of the 
main (exe) object file instead of the pdb. Up until now, that distinction was 
not important because `m_objfile_sp` was always the main file...



Comment at: 
lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:276-285
 if (!file_up) {
   auto module_sp = m_objfile_sp->GetModule();
   if (!module_sp)
 return 0;
   // See if any symbol file is specified through `--symfile` option.
   FileSpec symfile = module_sp->GetSymbolFileFileSpec();
   if (!symfile)

You should be able to delete this block now -- the code you're writing now 
supersedes (and enhances) it. If the module has a SymbolFileSpec, then this 
file will be loaded (as an ObjectFilePDB) in SymbolVendor::FindPlugin, and that 
instance will be passed to the SymbolFilePDB constructor.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:299
 std::unique_ptr file_up =
 loadMatchingPDBFile(m_objfile_sp->GetFileSpec().GetPath(), 
m_allocator);
 

zequanwu wrote:
> labath wrote:
> > Instead of changing `loadMatchingPDBFile` I think we ought to change this 
> > to something like:
> > ```
> > if (auto *pdb = llvm::dyn_cast(m_objfile_sp)) {
> >   file_up = pdb->giveMeAPDBFile(); 
> >   // PS: giveMeAPDBFile is a new pdb-specific public method on ObjectFilePDB
> >   // PS: possibly this should not return a PDBFile, but a PDBIndex directly 
> > -- I don't know the details but the general idea is that it could/should 
> > reuse the pdb parsing state that the objectfile class has already created
> > } else 
> >   file_up = do_the_loadMatchingPDBFile_fallback();
> > ```
> > 
> > The reason for that is that it avoids opening the pdb file twice (once in 
> > the object file and once here). Another reason is that I believe the entire 
> > `loadMatchingPDBFile` function should disappear. Finding the pdb file 
> > should not be the job of the symbol file class -- we have symbol vendors 
> > for that. However, we should keep the fallback for now to keep the patch 
> > small...
> From what I see, it seems like PDBFile is designed to be created when opening 
> the pdb file. It doesn't allow copying it around.
> `NativeSession` holds a unique_ptr of PDBFile and `NativeSession::getPDBFile` 
> returns the reference of object, so creating unique_ptr from it is not 
> allowed.
> 
> I moved `loadPDBFile` to `ObjectFilePDB` as static function, so we can move 
> `PDBFile` easily.
You don't need to copy it -- you can just have the two objects reference the 
same instance (stealing the PDBFile object from under ObjectFilePDB seems 
suboptimal). Probably the simplest way to achieve that is to change the 
PdbIndex class to use a raw pointer (and ObjectFilePDB to provide one). Then 
SymbolFilePDB can have an additional unique_ptr membe

[Lldb-commits] [PATCH] D89925: [lldb] Fix a regression introduced by D75730

[Pavel Labath via Phabricator via lldb-commits]

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

If needed, we can add some `#ifdef`s to produce suitable instructions on other 
architectures too.




Comment at: lldb/source/Core/Disassembler.cpp:544
const char *plugin_name, const char *flavor,
const ExecutionContext &exe_ctx,
uint32_t num_instructions,

Maybe change this to take a `StackFrame&` argument? The only caller 
StackFrame::Disassemble, so the frame argument is always valid (and a lot of 
this defensive code is not needed).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89925

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

[Lldb-commits] [PATCH] D89874: [lldb] Unify x86 watchpoint implementation on Linux and NetBSD (and future FreeBSD plugin) [WIP]

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

This looks very nice.

In D89874#2344759 , @mgorny wrote:

> In D89874#2344561 , @labath wrote:
>
>> I like this, and I think this is in a pretty good shape as it stands. The 
>> thing I'm not sure of is the naming of the class. I'd probably name it 
>> something like NativeRegisterContext_x86 (and hope that it can include 
>> non-watchpoint functionality in the future). As it stands now, this is not 
>> really a mix-in but a full register context class and in theory one can 
>> imagine that some target which only supports a single architecture will not 
>> bother with the NativeRegisterContextLinux stuff, but inherit straight from 
>> `NativeRegisterContext_x86`. (It's comment can allude to the fact that it is 
>> indented to be mixed with subclasses implementing os-specific functionality, 
>> which will also help explaining the virtual inheritance.)
>
> I don't have a strong opinion here. I went for the mixin approach since that 
> was your suggest wrt register caching. I suppose in this case it doesn't 
> matter much but it could make things easier (or harder) as we have more 
> potentially disjoint functions and partially transitioned source tree.

Ok, I see what you mean. We can keep this class watchpoint-specific, and 
re-evaluate later. I'd still like to have "NativeRegisterContext" in the name, 
though...

In D89874#2345649 , @mgorny wrote:

> Migrate NetBSD plugin as well.

It might be reasonable to land these as separate changes -- to reduce churn if 
anything breaks. Maybe start with NetBSD, since on linux you'll also have to 
update all NativeRegisterContextLinux_ARCH classes ?




Comment at: lldb/include/lldb/lldb-defines.h:62-65
+// Watchpoint types in gdb-remote protocol
+#define LLDB_GDB_WATCH_TYPE_WRITE 1
+#define LLDB_GDB_WATCH_TYPE_READ 2
+#define LLDB_GDB_WATCH_TYPE_READ_WRITE 3

I don't want to add any more defines than what we already have. The modern way 
to handle this would be via an enum. But that's best left for a separate patch. 
Let's just revert this here.



Comment at: 
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp:580
 
-  if (IsGPR(reg_index))
+  if (IsGPROrDR(reg_index))
 return WriteRegisterRaw(reg_index, reg_value);

krytarowski wrote:
> Can we have `IsGPR(reg_index) || IsDR(reg_index)`?
Yeah, that does sound better.



Comment at: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp:9
+
+#if defined(__i386__) || defined(__x86_64__)
+

mgorny wrote:
> labath wrote:
> > This class won't be using any os- or arch-specific features, so we can just 
> > drop these.
> Does it make sense to compile code that isn't going to be used on other 
> platforms?
It helps with maintenance because people can check if it at least compiles. For 
example, this change (making NativeRegisterContextLinux inherit virtually) will 
require changes in all of the NativeRegisterContextLinux_ARCH constructors, but 
those will have to be done blindly. We could even think of adding unit tests 
for this stuff, if we wanted to be super fancy...

At the same time, I don't think this hurts much because even the simplest form 
of dead code removal should be able to strip a file thats completely 
unreferenced.


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

https://reviews.llvm.org/D89874

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

[Lldb-commits] [PATCH] D89600: [lldb] Move copying of reproducer out of process

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D89600#2338996 , @JDevlieghere 
wrote:

> In D89600#2337868 , @labath wrote:
>
>> Hm... Are you sure that this will help with anything? IIUC, the 
>> serialization of the VFS file map still happens in the context of the 
>> crashing process. This requires walking a bunch of data structures with 
>> liberally-asserting code, which will abort on any inconsistency it runs 
>> into. If *that* succeeds, I'd expect that the copying of the files will 
>> succeed too.
>
> I only have a handful of crash logs to base myself off. Currently the walking 
> and the copying are intertwined, so I can't say for sure. My idea was to give 
> this a try and see where that got us and make more invasive changes (like 
> what you proposed) if needed. But since you sound pretty convinced that we'll 
> need it anyway I will extend the patch.

Well.. I'd expect that the reason why saving of the reproducer fails is because 
we corrupt the heap/stack before we finally crash. Copying files is basically a 
bunch of syscalls (though some of our abstractions get in the way), so it 
wouldn't normally be affected by this. Our ability to _find_ the files that 
we're supposed to copy is a different matter...

That said, this doesn't seem significantly more invasive than the previous 
approach.

>> I think it would be better if we moved the vfs map construction into the 
>> other process too. Maybe via something like this:
>>
>> - The first process opens a "log" file, which will contain the list of files 
>> to be copied. For added safety the file should be opened append-only.
>> - As it works, it writes file names to this file.
>> - Upon a crash, the re-execed process reads this file, constructs the vfs 
>> map, and copies all necessary files. It should be prepared to handle/ignore 
>> any garbled entries, and the format of the file should make this easy. 
>> (Since the log file must by definition contain enough information to 
>> recreate the vfs map, it might be simpler to not write out the map, but just 
>> always recreate it in memory when replaying.)
>
> Yes, that would be similar to how we immediately flush GDB remote packets to 
> disk. It would require adding some "immediate" mode to the FileCollector and 
> an API to convert between the formats. I think the new format could be just 
> host paths separated by newlines.

A nul character is better than a newline, as paths can contain newline 
characters.




Comment at: lldb/include/lldb/Utility/Reproducer.h:241-256
+class Finalizer {
+public:
+  Finalizer(Loader *loader) : m_loader(loader) {}
+  llvm::Error Finalize() const;
+
+protected:
+  Loader *m_loader;

This appears to be over-engineered. Is there a reason plain functions won't do?
```
llvm::Error FinalizeReproducer(Loader&);
llvm::Error FinalizeReproducerInProcess(FileSpec); // calls the first one
```
That said, I'm not sure "InProcess" captures the distinction very well, as both 
are "in process" at the point where they are used. Maybe name them according to 
what they do (as in, whether they generate a full reproducer, or one that needs 
to be fixed up afterwards)



Comment at: lldb/source/Utility/Reproducer.cpp:389
+
+  if (auto files = llvm::MemoryBuffer::getFile(files_path)) {
+SmallVector paths;

Is this deliberately ignoring a possible error?



Comment at: lldb/source/Utility/ReproducerProvider.cpp:53
+  std::error_code ec;
+  llvm::raw_fd_ostream os(m_files_path, ec, llvm::sys::fs::OF_Append);
+  if (ec)

So, this reopens the file every time a file is added? That is suboptimal... Why 
not just open it once and keep it that way? That would also enable handling of 
error during opening of files.



Comment at: lldb/source/Utility/ReproducerProvider.cpp:65-67
+  if (ec)
+return vfs->dir_begin(dir, dir_ec);
+  os << dir << "\n";

`if(!ec) os<(argv0);
-llvm::outs() << "\n";
-llvm::outs() << "Crash reproducer for ";
-llvm::outs() << lldb::SBDebugger::GetVersionString() << '\n';
-llvm::outs() << '\n';
-llvm::outs() << "Reproducer written to '" << SBReproducer::GetPath()
- << "'\n";
-llvm::outs() << '\n';
-llvm::outs() << "Before attaching the reproducer to a bug report:\n";
-llvm::outs() << " - Look at the directory to ensure you're willing to "
-"share its content.\n";
-llvm::outs()
-<< " - Make sure the reproducer works by replaying the reproducer.\n";
-llvm::outs() << '\n';
-llvm::outs() << "Replay the reproducer with the following command:\n";
-llvm::outs() << exe << " -replay " << SBReproducer::GetPath() << "\n";
-llvm::outs() << "\n";
+std::system(static_cast(cmd));
+fflush(stdout);

this is really cryptic. Maybe renami

[Lldb-commits] [lldb] bb1d702 - [lldb][NFC] Make GetShellSafeArgument return std::string and unittest it.

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2020-10-22T14:47:10+02:00
New Revision: bb1d702e25f5f23e8d5a755295f2921caaea2abb

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

LOG: [lldb][NFC] Make GetShellSafeArgument return std::string and unittest it.

Added: 


Modified: 
lldb/include/lldb/Utility/Args.h
lldb/source/Host/common/ProcessLaunchInfo.cpp
lldb/source/Utility/Args.cpp
lldb/unittests/Utility/ArgsTest.cpp

Removed: 




diff  --git a/lldb/include/lldb/Utility/Args.h 
b/lldb/include/lldb/Utility/Args.h
index 82e6d147ae56..b93677b53935 100644
--- a/lldb/include/lldb/Utility/Args.h
+++ b/lldb/include/lldb/Utility/Args.h
@@ -263,9 +263,8 @@ class Args {
 
   static uint32_t StringToGenericRegister(llvm::StringRef s);
 
-  static const char *GetShellSafeArgument(const FileSpec &shell,
-  const char *unsafe_arg,
-  std::string &safe_arg);
+  static std::string GetShellSafeArgument(const FileSpec &shell,
+  llvm::StringRef unsafe_arg);
 
   // EncodeEscapeSequences will change the textual representation of common
   // escape sequences like "\n" (two characters) into a single '\n'. It does

diff  --git a/lldb/source/Host/common/ProcessLaunchInfo.cpp 
b/lldb/source/Host/common/ProcessLaunchInfo.cpp
index 851069d0f20f..01cf3d76314e 100644
--- a/lldb/source/Host/common/ProcessLaunchInfo.cpp
+++ b/lldb/source/Host/common/ProcessLaunchInfo.cpp
@@ -253,7 +253,6 @@ bool ProcessLaunchInfo::ConvertArgumentsForLaunchingInShell(
   if (argv == nullptr || argv[0] == nullptr)
 return false;
   Args shell_arguments;
-  std::string safe_arg;
   shell_arguments.AppendArgument(shell_executable);
   const llvm::Triple &triple = GetArchitecture().GetTriple();
   if (triple.getOS() == llvm::Triple::Win32 &&
@@ -330,9 +329,10 @@ bool 
ProcessLaunchInfo::ConvertArgumentsForLaunchingInShell(
   return false;
   } else {
 for (size_t i = 0; argv[i] != nullptr; ++i) {
-  const char *arg =
-  Args::GetShellSafeArgument(m_shell, argv[i], safe_arg);
-  shell_command.Printf(" %s", arg);
+  std::string safe_arg = Args::GetShellSafeArgument(m_shell, argv[i]);
+  // Add a space to separate this arg from the previous one.
+  shell_command.PutCString(" ");
+  shell_command.PutCString(safe_arg);
 }
   }
   shell_arguments.AppendArgument(shell_command.GetString());

diff  --git a/lldb/source/Utility/Args.cpp b/lldb/source/Utility/Args.cpp
index 2cbe727ed240..152e73db08ea 100644
--- a/lldb/source/Utility/Args.cpp
+++ b/lldb/source/Utility/Args.cpp
@@ -379,9 +379,8 @@ void Args::Clear() {
   m_argv.push_back(nullptr);
 }
 
-const char *Args::GetShellSafeArgument(const FileSpec &shell,
-   const char *unsafe_arg,
-   std::string &safe_arg) {
+std::string Args::GetShellSafeArgument(const FileSpec &shell,
+   llvm::StringRef unsafe_arg) {
   struct ShellDescriptor {
 ConstString m_basename;
 const char *m_escapables;
@@ -403,7 +402,7 @@ const char *Args::GetShellSafeArgument(const FileSpec 
&shell,
 }
   }
 
-  safe_arg.assign(unsafe_arg);
+  std::string safe_arg(unsafe_arg);
   size_t prev_pos = 0;
   while (prev_pos < safe_arg.size()) {
 // Escape spaces and quotes
@@ -414,7 +413,7 @@ const char *Args::GetShellSafeArgument(const FileSpec 
&shell,
 } else
   break;
   }
-  return safe_arg.c_str();
+  return safe_arg;
 }
 
 lldb::Encoding Args::StringToEncoding(llvm::StringRef s,

diff  --git a/lldb/unittests/Utility/ArgsTest.cpp 
b/lldb/unittests/Utility/ArgsTest.cpp
index ed1235f99c71..cd62c08df31e 100644
--- a/lldb/unittests/Utility/ArgsTest.cpp
+++ b/lldb/unittests/Utility/ArgsTest.cpp
@@ -9,6 +9,7 @@
 #include "gtest/gtest.h"
 
 #include "lldb/Utility/Args.h"
+#include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/StringList.h"
 
 #include 
@@ -313,3 +314,25 @@ TEST(ArgsTest, Yaml) {
   EXPECT_EQ(entries[2].GetQuoteChar(), '"');
   EXPECT_EQ(entries[3].GetQuoteChar(), '\0');
 }
+
+TEST(ArgsTest, GetShellSafeArgument) {
+  // Try escaping with bash at start/middle/end of the argument.
+  FileSpec bash("/bin/bash", FileSpec::Style::posix);
+  EXPECT_EQ(Args::GetShellSafeArgument(bash, "\"b"), "\\\"b");
+  EXPECT_EQ(Args::GetShellSafeArgument(bash, "a\""), "a\\\"");
+  EXPECT_EQ(Args::GetShellSafeArgument(bash, "a\"b"), "a\\\"b");
+
+  // String that doesn't need to be escaped
+  EXPECT_EQ(Args::GetShellSafeArgument(bash, "a"), "a");
+
+  // Try escaping with tcsh and the tcsh-specific "$" escape.
+  FileSpec tcsh("/bin/tcsh", FileSpec::

[Lldb-commits] [PATCH] D89813: [CMake][NFC] Limit the use of uppercase_CMAKE_BUILD_TYPE

[Christopher Tetreault via Phabricator via lldb-commits]

ctetreau added a comment.

I would assume so. I'll take a look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89813

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

[Lldb-commits] [PATCH] D89813: [CMake][NFC] Limit the use of uppercase_CMAKE_BUILD_TYPE

[Louis Dionne via Phabricator via lldb-commits]

ldionne added a comment.

I quite like this. We should strive to use the same name as CMake uses for its 
build types, i.e. `Debug`, `Release`, etc.

If I configure CMake with `-DCMAKE_BUILD_TYPE=DEBUG`, is `CMAKE_BUILD_TYPE` 
normalized back to `Debug` by CMake, or is it defined to `DEBUG`? If the 
latter, then I suggest we might want to add a temporary error message when the 
`CMAKE_BUILD_TYPE` is defined to something all-uppercase. This would help catch 
cases where this patch will be a change in behavior. That would be a nice place 
to tell users to use the right build type names.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89813

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

[Lldb-commits] [PATCH] D89813: [CMake][NFC] Limit the use of uppercase_CMAKE_BUILD_TYPE

[Christopher Tetreault via Phabricator via lldb-commits]

ctetreau marked an inline comment as done.
ctetreau added a comment.

In D89813#2342463 , @ldionne wrote:

> I quite like this. We should strive to use the same name as CMake uses for 
> its build types, i.e. `Debug`, `Release`, etc.
>
> If I configure CMake with `-DCMAKE_BUILD_TYPE=DEBUG`, is `CMAKE_BUILD_TYPE` 
> normalized back to `Debug` by CMake, or is it defined to `DEBUG`? If the 
> latter, then I suggest we might want to add a temporary error message when 
> the `CMAKE_BUILD_TYPE` is defined to something all-uppercase. This would help 
> catch cases where this patch will be a change in behavior. That would be a 
> nice place to tell users to use the right build type names.

My interpretation of 
https://cmake.org/cmake/help/v3.13/variable/CMAKE_BUILD_TYPE.html, is that it 
can be the empty string, or CamelCase. However, experimentation shows that 
passing `-DCMAKE_BUILD_TYPE=RELEASE` results in an all-capitals version. Given 
this, I don't think this patch is correct.

It looks like we had case-sensitive CMAKE_BUILD_TYPE, but it was removed in 
2015 
(https://github.com/llvm/llvm-project/commit/74009be05158c72893e02c2930b771a334543f1b),
 so making it case sensitive again is probably going to be controversial.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89813

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

[Lldb-commits] [PATCH] D89813: [CMake][NFC] Limit the use of uppercase_CMAKE_BUILD_TYPE

[Christopher Tetreault via Phabricator via lldb-commits]

ctetreau created this revision.
Herald added subscribers: llvm-commits, libcxx-commits, lldb-commits, 
Sanitizers, lebedev.ri, hiraditya, arichardson, mgorny.
Herald added a reviewer: lebedev.ri.
Herald added projects: Sanitizers, LLDB, libc++, libc++abi, libunwind, LLVM.
Herald added a reviewer: libc++.
Herald added a reviewer: libc++abi.
Herald added a reviewer: libunwind.
ctetreau requested review of this revision.
Herald added a subscriber: JDevlieghere.

uppercase_CMAKE_BUILD_TYPE is often defined to be
toupper(CMAKE_BUILD_TYPE). This is sometimes neccesary when trying to
programatically inspect the values of builtin cmake variables that have
the build type as part of the name. However, this variable is also often
used to just check the build type.

The use of this variable is error prone because it is not a builtin variable.
Users may add a usage of it without defining it. Since CMake treats undefined
variables as being defined to the empty string, this will not cause any sort
of error. It may even be the case that it appears to work for the author
and bad code makes it into master. (D89807 )

This patch removes unneccesary usages of uppercase_CMAKE_BUILD_TYPE.
Remaining usages of it are needed to construct variable names such as
`CMAKE_CXX_FLAGS_DEBUG`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89813

Files:
  compiler-rt/cmake/Modules/AddCompilerRT.cmake
  libcxx/CMakeLists.txt
  libcxx/utils/google-benchmark/test/CMakeLists.txt
  libcxxabi/CMakeLists.txt
  libunwind/CMakeLists.txt
  lldb/CMakeLists.txt
  llvm/CMakeLists.txt
  llvm/cmake/modules/AddLLVM.cmake
  llvm/cmake/modules/AddOCaml.cmake
  llvm/cmake/modules/HandleLLVMOptions.cmake
  llvm/cmake/modules/TableGen.cmake
  llvm/lib/Support/CMakeLists.txt
  llvm/utils/benchmark/test/CMakeLists.txt

Index: llvm/utils/benchmark/test/CMakeLists.txt
===
--- llvm/utils/benchmark/test/CMakeLists.txt
+++ llvm/utils/benchmark/test/CMakeLists.txt
@@ -5,8 +5,7 @@
 
 # NOTE: Some tests use `` to perform the test. Therefore we must
 # strip -DNDEBUG from the default CMake flags in DEBUG mode.
-string(TOUPPER "${CMAKE_BUILD_TYPE}" uppercase_CMAKE_BUILD_TYPE)
-if( NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" )
+if( NOT CMAKE_BUILD_TYPE STREQUAL "Debug" )
   add_definitions( -UNDEBUG )
   add_definitions(-DTEST_BENCHMARK_LIBRARY_HAS_NO_ASSERTIONS)
   # Also remove /D NDEBUG to avoid MSVC warnings about conflicting defines.
Index: llvm/lib/Support/CMakeLists.txt
===
--- llvm/lib/Support/CMakeLists.txt
+++ llvm/lib/Support/CMakeLists.txt
@@ -50,6 +50,7 @@
 endif()
 
 # Override the C runtime allocator on Windows and embed it into LLVM tools & libraries
+string(TOUPPER "${CMAKE_BUILD_TYPE}" uppercase_CMAKE_BUILD_TYPE)
 if(LLVM_INTEGRATED_CRT_ALLOC)
   if (CMAKE_BUILD_TYPE AND NOT ${LLVM_USE_CRT_${uppercase_CMAKE_BUILD_TYPE}} MATCHES "^(MT|MTd)$")
 message(FATAL_ERROR "LLVM_INTEGRATED_CRT_ALLOC only works with /MT or /MTd. Use LLVM_USE_CRT_${uppercase_CMAKE_BUILD_TYPE} to set the appropriate option.")
Index: llvm/cmake/modules/TableGen.cmake
===
--- llvm/cmake/modules/TableGen.cmake
+++ llvm/cmake/modules/TableGen.cmake
@@ -54,8 +54,8 @@
   endif()
   # Comments are only useful for Debug builds. Omit them if the backend
   # supports it.
-  if (NOT (uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" OR
-   uppercase_CMAKE_BUILD_TYPE STREQUAL "RELWITHDEBINFO"))
+  if (NOT (CMAKE_BUILD_TYPE STREQUAL "Debug" OR
+   CMAKE_BUILD_TYPE STREQUAL "RelWithDebInfo"))
 list(FIND ARGN "-gen-dag-isel" idx)
 if (NOT idx EQUAL -1)
   list(APPEND LLVM_TABLEGEN_FLAGS "-omit-comments")
Index: llvm/cmake/modules/HandleLLVMOptions.cmake
===
--- llvm/cmake/modules/HandleLLVMOptions.cmake
+++ llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -2,10 +2,6 @@
 # options and executing the appropriate CMake commands to realize the users'
 # selections.
 
-# This is commonly needed so make sure it's defined before we include anything
-# else.
-string(TOUPPER "${CMAKE_BUILD_TYPE}" uppercase_CMAKE_BUILD_TYPE)
-
 include(CheckCompilerVersion)
 include(HandleLLVMStdlib)
 include(CheckCCompilerFlag)
@@ -58,7 +54,7 @@
   endif()
   # On non-Debug builds cmake automatically defines NDEBUG, so we
   # explicitly undefine it:
-  if( NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" )
+  if( NOT CMAKE_BUILD_TYPE STREQUAL "Debug" )
 # NOTE: use `add_compile_options` rather than `add_definitions` since
 # `add_definitions` does not support generator expressions.
 add_compile_options($<$,$>:-UNDEBUG>)
@@ -298,7 +294,7 @@
   endif()
   # GCC for MIPS can miscompile LLVM due to PR37701.
   if(CMAKE_COMPILER_IS_GNUCXX AND LLVM_NATIVE_ARCH STREQUAL "Mips" AND
- 

[Lldb-commits] [PATCH] D89813: [CMake][NFC] Limit the use of uppercase_CMAKE_BUILD_TYPE

[Christopher Tetreault via Phabricator via lldb-commits]

ctetreau added a comment.

In D89813#2342552 , @ctetreau wrote:

> I would assume so. I'll take a look.

It doesn't seem to work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89813

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

[Lldb-commits] [PATCH] D89813: [CMake][NFC] Limit the use of uppercase_CMAKE_BUILD_TYPE

[Louis Dionne via Phabricator via lldb-commits]

ldionne added a comment.

Are generator expressions expanded in `if()`? If so, we could at least change 
those to `if ("$" STREQUAL "DEBUG")`. This would 
preserve the case-insensitivity while solving the problem you were concerned 
about, and make the code easier to read.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89813

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

[Lldb-commits] [PATCH] D89842: [lldb/DWARF] Add support for DW_OP_implicit_value

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 299540.
mib added a comment.

Add test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89842

Files:
  lldb/source/Expression/DWARFExpression.cpp
  lldb/test/Shell/SymbolFile/DWARF/DW_OP_implicit_value.test
  lldb/test/Shell/SymbolFile/DWARF/Inputs/implicit_value.c


Index: lldb/test/Shell/SymbolFile/DWARF/Inputs/implicit_value.c
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/Inputs/implicit_value.c
@@ -0,0 +1,6 @@
+int main() {
+  double d = 3.14;
+  printf("break here");
+  d *= d;
+  return 0;
+}
Index: lldb/test/Shell/SymbolFile/DWARF/DW_OP_implicit_value.test
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/DW_OP_implicit_value.test
@@ -0,0 +1,19 @@
+# Make sure DW_OP_implicit_value is supported by lldb with DWARFv5
+# FIXME: Enable darwin platform once DWARFv5 is supported by dsymutil
+# rdar://67406091
+
+# REQUIRES: system-linux
+
+# RUN: %clang_host -gdwarf-5 -O1 %p/Inputs/implicit_value.c -o %t
+# RUN: %lldb -b -s %s %t | FileCheck %s
+
+br set -p 'break here'
+# CHECK: Breakpoint 1: where = {{.*}}`main {{.*}} at implicit_value.c:{{.*}}, 
address = 0x{{.*}}
+
+process launch
+# CHECK: break here
+
+frame var d
+# CHECK: (double) d = 3.1400{{.*}}
+
+quit
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -846,6 +846,26 @@
   return true;
 }
 
+static bool Evaluate_DW_OP_implicit_value(std::vector &stack,
+  ExecutionContext *exe_ctx,
+  RegisterContext *reg_ctx,
+  const DataExtractor &opcodes,
+  lldb::offset_t &opcode_offset,
+  Status *error_ptr, Log *log) {
+
+  const uint32_t len = opcodes.GetULEB128(&opcode_offset);
+  const void *data = opcodes.GetData(&opcode_offset, len);
+
+  if (!data) {
+LLDB_LOG(log, "Evaluate_DW_OP_implicit_value: could not be read data");
+return false;
+  }
+
+  Value result(data, len);
+  stack.push_back(result);
+  return true;
+}
+
 bool DWARFExpression::Evaluate(ExecutionContextScope *exe_scope,
lldb::addr_t loclist_base_load_addr,
const Value *initial_value_ptr,
@@ -2248,6 +2268,23 @@
   }
   break;
 
+// OPCODE: DW_OP_implicit_value
+// OPERANDS: 2
+//  ULEB128  size of the value block in bytes
+//  uint8_t* block bytes encoding value in target's memory
+//  representation
+// DESCRIPTION: Value is immediately stored in block in the debug info with
+// the memory representation of the target.
+case DW_OP_implicit_value: {
+  if (!Evaluate_DW_OP_implicit_value(stack, exe_ctx, reg_ctx, opcodes,
+ offset, error_ptr, log)) {
+LLDB_ERRORF(error_ptr, "Could not evaluate %s.",
+DW_OP_value_to_name(op));
+return false;
+  }
+  break;
+}
+
 // OPCODE: DW_OP_push_object_address
 // OPERANDS: none
 // DESCRIPTION: Pushes the address of the object currently being


Index: lldb/test/Shell/SymbolFile/DWARF/Inputs/implicit_value.c
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/Inputs/implicit_value.c
@@ -0,0 +1,6 @@
+int main() {
+  double d = 3.14;
+  printf("break here");
+  d *= d;
+  return 0;
+}
Index: lldb/test/Shell/SymbolFile/DWARF/DW_OP_implicit_value.test
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/DW_OP_implicit_value.test
@@ -0,0 +1,19 @@
+# Make sure DW_OP_implicit_value is supported by lldb with DWARFv5
+# FIXME: Enable darwin platform once DWARFv5 is supported by dsymutil
+# rdar://67406091
+
+# REQUIRES: system-linux
+
+# RUN: %clang_host -gdwarf-5 -O1 %p/Inputs/implicit_value.c -o %t
+# RUN: %lldb -b -s %s %t | FileCheck %s
+
+br set -p 'break here'
+# CHECK: Breakpoint 1: where = {{.*}}`main {{.*}} at implicit_value.c:{{.*}}, address = 0x{{.*}}
+
+process launch
+# CHECK: break here
+
+frame var d
+# CHECK: (double) d = 3.1400{{.*}}
+
+quit
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -846,6 +846,26 @@
   return true;
 }
 
+static bool Evaluate_DW_OP_implicit_value(std::vector &stack,
+  ExecutionContext *exe_ctx,
+  RegisterCont

[Lldb-commits] [PATCH] D89842: [lldb/DWARF] Add support DW_OP_implicit_value

[Med Ismail Bennani via Phabricator via lldb-commits]

mib created this revision.
mib added reviewers: labath, JDevlieghere, aprantl.
mib added projects: LLDB, debug-info.
Herald added a subscriber: lldb-commits.
mib requested review of this revision.

This patch completes https://reviews.llvm.org/D83560. Now that the
compiler can emit `DW_OP_implicit_value` into DWARF expressions, lldb
needed to learn to read these opcodes for variable inspection and
expression evaluation.

This implicit location descriptor specifies an immediate value with two
operands: a length (ULEB128) followed by a block representing the value
in the target memory representation.

Note that, at the moment, this feature is only supported when emitting
DWARFv5 debugging information. However, since dsymutil doesn't support
that version yet, the test is disabled on macOS and only ran in Linux.

rdar://67406091

Signed-off-by: Med Ismail Bennani 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89842

Files:
  lldb/source/Expression/DWARFExpression.cpp
  lldb/test/Shell/SymbolFile/DWARF/Inputs/implicit_value.c


Index: lldb/test/Shell/SymbolFile/DWARF/Inputs/implicit_value.c
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/Inputs/implicit_value.c
@@ -0,0 +1,6 @@
+int main() {
+  double d = 3.14;
+  printf("break here");
+  d *= d;
+  return 0;
+}
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -846,6 +846,26 @@
   return true;
 }
 
+static bool Evaluate_DW_OP_implicit_value(std::vector &stack,
+  ExecutionContext *exe_ctx,
+  RegisterContext *reg_ctx,
+  const DataExtractor &opcodes,
+  lldb::offset_t &opcode_offset,
+  Status *error_ptr, Log *log) {
+
+  const uint32_t len = opcodes.GetULEB128(&opcode_offset);
+  const void *data = opcodes.GetData(&opcode_offset, len);
+
+  if (!data) {
+LLDB_LOG(log, "Evaluate_DW_OP_implicit_value: could not be read data");
+return false;
+  }
+
+  Value result(data, len);
+  stack.push_back(result);
+  return true;
+}
+
 bool DWARFExpression::Evaluate(ExecutionContextScope *exe_scope,
lldb::addr_t loclist_base_load_addr,
const Value *initial_value_ptr,
@@ -2248,6 +2268,23 @@
   }
   break;
 
+// OPCODE: DW_OP_implicit_value
+// OPERANDS: 2
+//  ULEB128  size of the value block in bytes
+//  uint8_t* block bytes encoding value in target's memory
+//  representation
+// DESCRIPTION: Value is immediately stored in block in the debug info with
+// the memory representation of the target.
+case DW_OP_implicit_value: {
+  if (!Evaluate_DW_OP_implicit_value(stack, exe_ctx, reg_ctx, opcodes,
+ offset, error_ptr, log)) {
+LLDB_ERRORF(error_ptr, "Could not evaluate %s.",
+DW_OP_value_to_name(op));
+return false;
+  }
+  break;
+}
+
 // OPCODE: DW_OP_push_object_address
 // OPERANDS: none
 // DESCRIPTION: Pushes the address of the object currently being


Index: lldb/test/Shell/SymbolFile/DWARF/Inputs/implicit_value.c
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/Inputs/implicit_value.c
@@ -0,0 +1,6 @@
+int main() {
+  double d = 3.14;
+  printf("break here");
+  d *= d;
+  return 0;
+}
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -846,6 +846,26 @@
   return true;
 }
 
+static bool Evaluate_DW_OP_implicit_value(std::vector &stack,
+  ExecutionContext *exe_ctx,
+  RegisterContext *reg_ctx,
+  const DataExtractor &opcodes,
+  lldb::offset_t &opcode_offset,
+  Status *error_ptr, Log *log) {
+
+  const uint32_t len = opcodes.GetULEB128(&opcode_offset);
+  const void *data = opcodes.GetData(&opcode_offset, len);
+
+  if (!data) {
+LLDB_LOG(log, "Evaluate_DW_OP_implicit_value: could not be read data");
+return false;
+  }
+
+  Value result(data, len);
+  stack.push_back(result);
+  return true;
+}
+
 bool DWARFExpression::Evaluate(ExecutionContextScope *exe_scope,
lldb::addr_t loclist_base_load_addr,
const Value *initial_value_ptr,
@@ -2248,6 +2268,23 @@
   }
   break;
 
+// OPCODE: DW_

[Lldb-commits] [PATCH] D89813: [CMake][NFC] Limit the use of uppercase_CMAKE_BUILD_TYPE

[Roman Lebedev via Phabricator via lldb-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added inline comments.
This revision now requires changes to proceed.



Comment at: libcxx/utils/google-benchmark/test/CMakeLists.txt:8-9
 # strip -DNDEBUG from the default CMake flags in DEBUG mode.
-string(TOUPPER "${CMAKE_BUILD_TYPE}" uppercase_CMAKE_BUILD_TYPE)
-if( NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" )
   add_definitions( -UNDEBUG )

How is this an unnecessary usage?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89813

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

[Lldb-commits] [PATCH] D89842: [lldb/DWARF] Add support for DW_OP_implicit_value

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 299539.
mib retitled this revision from "[lldb/DWARF] Add support DW_OP_implicit_value" 
to "[lldb/DWARF] Add support for DW_OP_implicit_value".
mib edited the summary of this revision.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89842

Files:
  lldb/source/Expression/DWARFExpression.cpp
  lldb/test/Shell/SymbolFile/DWARF/Inputs/implicit_value.c


Index: lldb/test/Shell/SymbolFile/DWARF/Inputs/implicit_value.c
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/Inputs/implicit_value.c
@@ -0,0 +1,6 @@
+int main() {
+  double d = 3.14;
+  printf("break here");
+  d *= d;
+  return 0;
+}
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -846,6 +846,26 @@
   return true;
 }
 
+static bool Evaluate_DW_OP_implicit_value(std::vector &stack,
+  ExecutionContext *exe_ctx,
+  RegisterContext *reg_ctx,
+  const DataExtractor &opcodes,
+  lldb::offset_t &opcode_offset,
+  Status *error_ptr, Log *log) {
+
+  const uint32_t len = opcodes.GetULEB128(&opcode_offset);
+  const void *data = opcodes.GetData(&opcode_offset, len);
+
+  if (!data) {
+LLDB_LOG(log, "Evaluate_DW_OP_implicit_value: could not be read data");
+return false;
+  }
+
+  Value result(data, len);
+  stack.push_back(result);
+  return true;
+}
+
 bool DWARFExpression::Evaluate(ExecutionContextScope *exe_scope,
lldb::addr_t loclist_base_load_addr,
const Value *initial_value_ptr,
@@ -2248,6 +2268,23 @@
   }
   break;
 
+// OPCODE: DW_OP_implicit_value
+// OPERANDS: 2
+//  ULEB128  size of the value block in bytes
+//  uint8_t* block bytes encoding value in target's memory
+//  representation
+// DESCRIPTION: Value is immediately stored in block in the debug info with
+// the memory representation of the target.
+case DW_OP_implicit_value: {
+  if (!Evaluate_DW_OP_implicit_value(stack, exe_ctx, reg_ctx, opcodes,
+ offset, error_ptr, log)) {
+LLDB_ERRORF(error_ptr, "Could not evaluate %s.",
+DW_OP_value_to_name(op));
+return false;
+  }
+  break;
+}
+
 // OPCODE: DW_OP_push_object_address
 // OPERANDS: none
 // DESCRIPTION: Pushes the address of the object currently being


Index: lldb/test/Shell/SymbolFile/DWARF/Inputs/implicit_value.c
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/Inputs/implicit_value.c
@@ -0,0 +1,6 @@
+int main() {
+  double d = 3.14;
+  printf("break here");
+  d *= d;
+  return 0;
+}
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -846,6 +846,26 @@
   return true;
 }
 
+static bool Evaluate_DW_OP_implicit_value(std::vector &stack,
+  ExecutionContext *exe_ctx,
+  RegisterContext *reg_ctx,
+  const DataExtractor &opcodes,
+  lldb::offset_t &opcode_offset,
+  Status *error_ptr, Log *log) {
+
+  const uint32_t len = opcodes.GetULEB128(&opcode_offset);
+  const void *data = opcodes.GetData(&opcode_offset, len);
+
+  if (!data) {
+LLDB_LOG(log, "Evaluate_DW_OP_implicit_value: could not be read data");
+return false;
+  }
+
+  Value result(data, len);
+  stack.push_back(result);
+  return true;
+}
+
 bool DWARFExpression::Evaluate(ExecutionContextScope *exe_scope,
lldb::addr_t loclist_base_load_addr,
const Value *initial_value_ptr,
@@ -2248,6 +2268,23 @@
   }
   break;
 
+// OPCODE: DW_OP_implicit_value
+// OPERANDS: 2
+//  ULEB128  size of the value block in bytes
+//  uint8_t* block bytes encoding value in target's memory
+//  representation
+// DESCRIPTION: Value is immediately stored in block in the debug info with
+// the memory representation of the target.
+case DW_OP_implicit_value: {
+  if (!Evaluate_DW_OP_implicit_value(stack, exe_ctx, reg_ctx, opcodes,
+ offset, error_ptr, log)) {
+LLDB_ERRORF(error_ptr, "Could not evaluate %s.",
+DW_OP_value

[Lldb-commits] [PATCH] D89842: [lldb/DWARF] Add support for DW_OP_implicit_value

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The code looks pretty straight-forward, but I believe the test should be 
asm-based with hardcoded dwarf (if you're feeling adventurous, you can also try 
yaml). My reasons for that are:

- it makes it guarantees (and makes it explicit) the input that is being tested
- the test can run on any platform (if done right -- no launching of processes)
- it enables us to test the error scenario where we fail to read the 
DW_OP_implicit_value data.

I think the simplest way to produce such a test would be to take 
`DW_TAG_variable-DW_AT_const_value.s` and exchange DW_AT_const_value for 
DW_AT_location/DW_FORM_exprloc




Comment at: lldb/source/Expression/DWARFExpression.cpp:856-867
+  const uint32_t len = opcodes.GetULEB128(&opcode_offset);
+  const void *data = opcodes.GetData(&opcode_offset, len);
+
+  if (!data) {
+LLDB_LOG(log, "Evaluate_DW_OP_implicit_value: could not be read data");
+return false;
+  }

I'm not sure this function is really complex enough to justify its existence. 
The actual code is pretty short and most of it is just argument lists and 
logging. I don't think the logging is very useful as the caller logs already, 
and the argument lists would go away if this were inlined.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89842

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

[Lldb-commits] [PATCH] D89842: [lldb/DWARF] Add support for DW_OP_implicit_value

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 299941.
mib edited the summary of this revision.
mib added a comment.

Change shell test for unit test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89842

Files:
  lldb/source/Expression/DWARFExpression.cpp
  lldb/unittests/Expression/DWARFExpressionTest.cpp


Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -218,6 +218,14 @@
   llvm::HasValue(GetScalar(16, 0xff00, true)));
 }
 
+TEST(DWARFExpression, DW_OP_implicit_value) {
+  unsigned char bytes = 4;
+
+  EXPECT_THAT_EXPECTED(
+  Evaluate({DW_OP_implicit_value, bytes, 0x11, 0x22, 0x33, 0x44}),
+  llvm::HasValue(GetScalar(8 * bytes, 0x44332211, true)));
+}
+
 TEST(DWARFExpression, DW_OP_unknown) {
   EXPECT_THAT_EXPECTED(
   Evaluate({0xff}),
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -2248,6 +2248,29 @@
   }
   break;
 
+// OPCODE: DW_OP_implicit_value
+// OPERANDS: 2
+//  ULEB128  size of the value block in bytes
+//  uint8_t* block bytes encoding value in target's memory
+//  representation
+// DESCRIPTION: Value is immediately stored in block in the debug info with
+// the memory representation of the target.
+case DW_OP_implicit_value: {
+  const uint32_t len = opcodes.GetULEB128(&offset);
+  const void *data = opcodes.GetData(&offset, len);
+
+  if (!data) {
+LLDB_LOG(log, "Evaluate_DW_OP_implicit_value: could not be read data");
+LLDB_ERRORF(error_ptr, "Could not evaluate %s.",
+DW_OP_value_to_name(op));
+return false;
+  }
+
+  Value result(data, len);
+  stack.push_back(result);
+  break;
+}
+
 // OPCODE: DW_OP_push_object_address
 // OPERANDS: none
 // DESCRIPTION: Pushes the address of the object currently being


Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -218,6 +218,14 @@
   llvm::HasValue(GetScalar(16, 0xff00, true)));
 }
 
+TEST(DWARFExpression, DW_OP_implicit_value) {
+  unsigned char bytes = 4;
+
+  EXPECT_THAT_EXPECTED(
+  Evaluate({DW_OP_implicit_value, bytes, 0x11, 0x22, 0x33, 0x44}),
+  llvm::HasValue(GetScalar(8 * bytes, 0x44332211, true)));
+}
+
 TEST(DWARFExpression, DW_OP_unknown) {
   EXPECT_THAT_EXPECTED(
   Evaluate({0xff}),
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -2248,6 +2248,29 @@
   }
   break;
 
+// OPCODE: DW_OP_implicit_value
+// OPERANDS: 2
+//  ULEB128  size of the value block in bytes
+//  uint8_t* block bytes encoding value in target's memory
+//  representation
+// DESCRIPTION: Value is immediately stored in block in the debug info with
+// the memory representation of the target.
+case DW_OP_implicit_value: {
+  const uint32_t len = opcodes.GetULEB128(&offset);
+  const void *data = opcodes.GetData(&offset, len);
+
+  if (!data) {
+LLDB_LOG(log, "Evaluate_DW_OP_implicit_value: could not be read data");
+LLDB_ERRORF(error_ptr, "Could not evaluate %s.",
+DW_OP_value_to_name(op));
+return false;
+  }
+
+  Value result(data, len);
+  stack.push_back(result);
+  break;
+}
+
 // OPCODE: DW_OP_push_object_address
 // OPERANDS: none
 // DESCRIPTION: Pushes the address of the object currently being
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D89842: [lldb/DWARF] Add support for DW_OP_implicit_value

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D89842#2345947 , @aprantl wrote:

> Perhaps the test fits in here 
> https://github.com/llvm/llvm-project/blob/master/lldb/unittests/Expression/DWARFExpressionTest.cpp
>  ?

Yes, that's even better. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89842

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

[Lldb-commits] [PATCH] D89842: [lldb/DWARF] Add support for DW_OP_implicit_value

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Perhaps the test fits in here 
https://github.com/llvm/llvm-project/blob/master/lldb/unittests/Expression/DWARFExpressionTest.cpp
 ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89842

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

[Lldb-commits] [PATCH] D89842: [lldb/DWARF] Add support for DW_OP_implicit_value

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

+1 on everything Pavel said.




Comment at: lldb/source/Expression/DWARFExpression.cpp:856-867
+  const uint32_t len = opcodes.GetULEB128(&opcode_offset);
+  const void *data = opcodes.GetData(&opcode_offset, len);
+
+  if (!data) {
+LLDB_LOG(log, "Evaluate_DW_OP_implicit_value: could not be read data");
+return false;
+  }

labath wrote:
> I'm not sure this function is really complex enough to justify its existence. 
> The actual code is pretty short and most of it is just argument lists and 
> logging. I don't think the logging is very useful as the caller logs already, 
> and the argument lists would go away if this were inlined.
Agreed, plus half of the arguments are unused. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89842

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

[Lldb-commits] [lldb] 4118522 - [lldb] Explicitly use the configuration architecture when building test executables

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2020-10-22T15:30:25+02:00
New Revision: 41185226f6d80663b4a1064c6f47581ee567d78d

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

LOG: [lldb] Explicitly use the configuration architecture when building test 
executables

The Darwin builder currently assumes in `getArchCFlags` that the passed `arch`
value is an actual string it can string.join with vendor/os/version/env strings:

```
   triple = '-'.join([arch, vendor, os, version, env])
```

However this is not true for most tests as we just pass down the `arch=None`
default value from `TestBase.build`. This causes that if we actually end up in
this function we just error out when concatenating `None` with the other actual
strings of vendor/os/version/env. What we should do instead is check that if
there is no test-specific architecture that we fall back to the configuration's
architecture value.

It seems we already worked around this in `builder.getArchSpec` by explicitly
falling back to the architecture specified in the configuration.

This patch just moves this fallback logic to the top `build` function so that it
affects all functions called from `TestBase.build`.

Reviewed By: JDevlieghere

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

Added: 


Modified: 
lldb/packages/Python/lldbsuite/test/builders/builder.py
lldb/packages/Python/lldbsuite/test/lldbtest.py

Removed: 




diff  --git a/lldb/packages/Python/lldbsuite/test/builders/builder.py 
b/lldb/packages/Python/lldbsuite/test/builders/builder.py
index fbfa86700e22..6c9584224f4a 100644
--- a/lldb/packages/Python/lldbsuite/test/builders/builder.py
+++ b/lldb/packages/Python/lldbsuite/test/builders/builder.py
@@ -93,11 +93,7 @@ def getArchSpec(self, architecture):
 Helper function to return the key-value string to specify the 
architecture
 used for the make system.
 """
-arch = architecture if architecture else None
-if not arch and configuration.arch:
-arch = configuration.arch
-
-return ("ARCH=" + arch) if arch else ""
+return ("ARCH=" + architecture) if architecture else ""
 
 def getCCSpec(self, compiler):
 """

diff  --git a/lldb/packages/Python/lldbsuite/test/lldbtest.py 
b/lldb/packages/Python/lldbsuite/test/lldbtest.py
index 69da3914f1f2..f469ce86f26e 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -2606,6 +2606,9 @@ def build(
 """Platform specific way to build the default binaries."""
 module = builder_module()
 
+if not architecture and configuration.arch:
+architecture = configuration.arch
+
 dictionary = lldbplatformutil.finalize_build_dictionary(dictionary)
 if self.getDebugInfo() is None:
 return self.buildDefault(architecture, compiler, dictionary)



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

[Lldb-commits] [PATCH] D89056: [lldb] Explicitly use the configuration architecture when building test executables

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG41185226f6d8: [lldb] Explicitly use the configuration 
architecture when building test… (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/D89056/new/

https://reviews.llvm.org/D89056

Files:
  lldb/packages/Python/lldbsuite/test/builders/builder.py
  lldb/packages/Python/lldbsuite/test/lldbtest.py


Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -2606,6 +2606,9 @@
 """Platform specific way to build the default binaries."""
 module = builder_module()
 
+if not architecture and configuration.arch:
+architecture = configuration.arch
+
 dictionary = lldbplatformutil.finalize_build_dictionary(dictionary)
 if self.getDebugInfo() is None:
 return self.buildDefault(architecture, compiler, dictionary)
Index: lldb/packages/Python/lldbsuite/test/builders/builder.py
===
--- lldb/packages/Python/lldbsuite/test/builders/builder.py
+++ lldb/packages/Python/lldbsuite/test/builders/builder.py
@@ -93,11 +93,7 @@
 Helper function to return the key-value string to specify the 
architecture
 used for the make system.
 """
-arch = architecture if architecture else None
-if not arch and configuration.arch:
-arch = configuration.arch
-
-return ("ARCH=" + arch) if arch else ""
+return ("ARCH=" + architecture) if architecture else ""
 
 def getCCSpec(self, compiler):
 """


Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -2606,6 +2606,9 @@
 """Platform specific way to build the default binaries."""
 module = builder_module()
 
+if not architecture and configuration.arch:
+architecture = configuration.arch
+
 dictionary = lldbplatformutil.finalize_build_dictionary(dictionary)
 if self.getDebugInfo() is None:
 return self.buildDefault(architecture, compiler, dictionary)
Index: lldb/packages/Python/lldbsuite/test/builders/builder.py
===
--- lldb/packages/Python/lldbsuite/test/builders/builder.py
+++ lldb/packages/Python/lldbsuite/test/builders/builder.py
@@ -93,11 +93,7 @@
 Helper function to return the key-value string to specify the architecture
 used for the make system.
 """
-arch = architecture if architecture else None
-if not arch and configuration.arch:
-arch = configuration.arch
-
-return ("ARCH=" + arch) if arch else ""
+return ("ARCH=" + architecture) if architecture else ""
 
 def getCCSpec(self, compiler):
 """
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D89874: [lldb] Split out NetBSD/x86 watchpoint impl for unification

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 299950.
mgorny retitled this revision from "[lldb] Unify x86 watchpoint implementation 
on Linux and NetBSD (and future FreeBSD plugin) [WIP]" to "[lldb] Split out 
NetBSD/x86 watchpoint impl for unification".
mgorny edited the summary of this revision.
mgorny added a comment.
Herald added a subscriber: arichardson.

Updated per comments, made it NetBSD-only for now.


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

https://reviews.llvm.org/D89874

Files:
  lldb/include/lldb/Host/common/NativeRegisterContext.h
  lldb/source/Host/common/NativeRegisterContext.cpp
  lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD.cpp
  lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD.h
  lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
  lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.h
  lldb/source/Plugins/Process/Utility/CMakeLists.txt
  lldb/source/Plugins/Process/Utility/NativeRegisterContextWatchpoint_x86.cpp
  lldb/source/Plugins/Process/Utility/NativeRegisterContextWatchpoint_x86.h

Index: lldb/source/Plugins/Process/Utility/NativeRegisterContextWatchpoint_x86.h
===
--- /dev/null
+++ lldb/source/Plugins/Process/Utility/NativeRegisterContextWatchpoint_x86.h
@@ -0,0 +1,48 @@
+//===-- NativeRegisterContextWatchpoint_x86.h ---*- 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
+//
+//===--===//
+
+#ifndef lldb_NativeRegisterContextWatchpoint_x86_h
+#define lldb_NativeRegisterContextWatchpoint_x86_h
+
+#include "Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h"
+
+namespace lldb_private {
+
+class NativeRegisterContextWatchpoint_x86 : public virtual NativeRegisterContextRegisterInfo {
+public:
+  Status IsWatchpointHit(uint32_t wp_index, bool &is_hit) override;
+
+  Status GetWatchpointHitIndex(uint32_t &wp_index,
+   lldb::addr_t trap_addr) override;
+
+  Status IsWatchpointVacant(uint32_t wp_index, bool &is_vacant) override;
+
+  bool ClearHardwareWatchpoint(uint32_t wp_index) override;
+
+  Status ClearWatchpointHit(uint32_t wp_index) override;
+
+  Status ClearAllHardwareWatchpoints() override;
+
+  Status SetHardwareWatchpointWithIndex(lldb::addr_t addr, size_t size,
+uint32_t watch_flags,
+uint32_t wp_index);
+
+  uint32_t SetHardwareWatchpoint(lldb::addr_t addr, size_t size,
+ uint32_t watch_flags) override;
+
+  lldb::addr_t GetWatchpointAddress(uint32_t wp_index) override;
+
+  uint32_t NumSupportedHardwareWatchpoints() override;
+
+private:
+  const RegisterInfo *GetDR(int num) const;
+};
+
+} // namespace lldb_private
+
+#endif // #ifndef lldb_NativeRegisterContextWatchpoint_x86_h
Index: lldb/source/Plugins/Process/Utility/NativeRegisterContextWatchpoint_x86.cpp
===
--- /dev/null
+++ lldb/source/Plugins/Process/Utility/NativeRegisterContextWatchpoint_x86.cpp
@@ -0,0 +1,265 @@
+//===-- NativeRegisterContextWatchpoint_x86.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "NativeRegisterContextWatchpoint_x86.h"
+
+#include "lldb/Utility/Log.h"
+#include "lldb/Utility/RegisterValue.h"
+
+#include "Plugins/Process/Utility/lldb-x86-register-enums.h"
+
+using namespace lldb_private;
+
+// Returns mask/value for status bit of wp_index in DR6
+static inline uint64_t GetStatusBit(uint32_t wp_index) {
+  // DR6: ...
+  // 3210 <- status bits for bp./wp. i; 1 if hit
+  return 1 << wp_index;
+}
+
+// Returns mask/value for global enable bit of wp_index in DR7
+static inline uint64_t GetEnableBit(uint32_t wp_index) {
+  // DR7: ...GLGLGLGL
+  // 33221100 <- global/local enable for bp./wp.; 1 if enabled
+  // we use global bits because NetBSD kernel does not preserve local
+  // bits reliably; Linux seems fine with either
+  return 1 << (2 * wp_index + 1);
+}
+
+// Returns mask for both enable bits of wp_index in DR7
+static inline uint64_t GetBothEnableBitMask(uint32_t wp_index) {
+  // DR7: ...GLGLGLGL
+  // 33221100 <- global/local enable for bp./wp.; 1 if enabled
+  return 3 << (2 * wp_index + 1);
+}
+
+// Returns value for type bits of wp_index in DR7
+static inline uint64_t GetWatchTypeBits(uint32_t watch_flags, uint32_t wp_index) {
+  // DR7:
+  // bit

[Lldb-commits] [PATCH] D89698: [lldb] Fix TestTargetAPI.py on Apple simulators

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG30d5590d171c: [lldb] Fix TestTargetAPI.py on Apple 
simulators (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/D89698/new/

https://reviews.llvm.org/D89698

Files:
  lldb/test/API/python_api/target/TestTargetAPI.py


Index: lldb/test/API/python_api/target/TestTargetAPI.py
===
--- lldb/test/API/python_api/target/TestTargetAPI.py
+++ lldb/test/API/python_api/target/TestTargetAPI.py
@@ -331,7 +331,7 @@
 if not desc:
 self.fail("SBTarget.GetDescription() failed")
 self.expect(desc, exe=False,
-substrs=['a.out', 'Target', 'Module', 'Breakpoint'])
+substrs=['Target', 'Module', 'a.out', 'Breakpoint'])
 
 @not_remote_testsuite_ready
 @add_test_categories(['pyapi'])


Index: lldb/test/API/python_api/target/TestTargetAPI.py
===
--- lldb/test/API/python_api/target/TestTargetAPI.py
+++ lldb/test/API/python_api/target/TestTargetAPI.py
@@ -331,7 +331,7 @@
 if not desc:
 self.fail("SBTarget.GetDescription() failed")
 self.expect(desc, exe=False,
-substrs=['a.out', 'Target', 'Module', 'Breakpoint'])
+substrs=['Target', 'Module', 'a.out', 'Breakpoint'])
 
 @not_remote_testsuite_ready
 @add_test_categories(['pyapi'])
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 30d5590 - [lldb] Fix TestTargetAPI.py on Apple simulators

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2020-10-22T16:41:54+02:00
New Revision: 30d5590d171c40e05b65585d1b531d8489e783e2

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

LOG: [lldb] Fix TestTargetAPI.py on Apple simulators

This test checks that the output of `SBTarget.GetDescription()` contains the
substrings `'a.out', 'Target', 'Module', 'Breakpoint'` in that order. This test
is currently failing on Apple simulators as apparently 'Module' can't be found
in the output after 'Target".

The reason for that is that the actual output of `SBTarget.GetDescription()` 
looks like this:
```
Target
  Module 
/build/path/lldb-test-build.noindex/python_api/target/TestTargetAPI.test_get_description_dwarf/a.out
0x7ff2b6d3f990: ObjectFileMachO64, file = 
/build/path/lldb-test-build.noindex/python_api/target/TestTargetAPI.test_get_description
[...]
0x7ff30715:   BreakpointList with 0 Breakpoints:

```

Clearly the string order should be `'Target', 'Module', 'a.out', 'Breakpoint'`.
However, LLDB is also a bunch of system shared libraries (libxpc.dylib,
libobjc.A.dylib, etc.) when *not* running against a simulator, we end up
unintentionally finding the `'Target', 'Module', 'Breakpoint'` substrings in the
trailing descriptions of the system modules. When running against a simulator we
however don't load shared system libraries.

This patch just moves the substrings in the correct order to make this test pass
without having any shared library modules in the description output.

Reviewed By: JDevlieghere

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

Added: 


Modified: 
lldb/test/API/python_api/target/TestTargetAPI.py

Removed: 




diff  --git a/lldb/test/API/python_api/target/TestTargetAPI.py 
b/lldb/test/API/python_api/target/TestTargetAPI.py
index a4d3aea94bec..0f5fe80177e6 100644
--- a/lldb/test/API/python_api/target/TestTargetAPI.py
+++ b/lldb/test/API/python_api/target/TestTargetAPI.py
@@ -331,7 +331,7 @@ def get_description(self):
 if not desc:
 self.fail("SBTarget.GetDescription() failed")
 self.expect(desc, exe=False,
-substrs=['a.out', 'Target', 'Module', 'Breakpoint'])
+substrs=['Target', 'Module', 'a.out', 'Breakpoint'])
 
 @not_remote_testsuite_ready
 @add_test_categories(['pyapi'])



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

[Lldb-commits] [PATCH] D89925: [lldb] Fix a regression introduced by D75730

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG826997c46280: [lldb] Fix a regression introduced by D75730 
(authored by JDevlieghere).

Changed prior to commit:
  https://reviews.llvm.org/D89925?vs=299853&id=22#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89925

Files:
  lldb/include/lldb/Core/Disassembler.h
  lldb/source/Core/Disassembler.cpp
  lldb/source/Target/StackFrame.cpp
  lldb/test/API/commands/disassemble/basic/TestFrameDisassemble.py
  lldb/test/API/commands/disassemble/basic/main.cpp

Index: lldb/test/API/commands/disassemble/basic/main.cpp
===
--- lldb/test/API/commands/disassemble/basic/main.cpp
+++ lldb/test/API/commands/disassemble/basic/main.cpp
@@ -2,6 +2,7 @@
 sum (int a, int b)
 {
 int result = a + b; // Set a breakpoint here
+asm("nop");
 return result;
 }
 
Index: lldb/test/API/commands/disassemble/basic/TestFrameDisassemble.py
===
--- lldb/test/API/commands/disassemble/basic/TestFrameDisassemble.py
+++ lldb/test/API/commands/disassemble/basic/TestFrameDisassemble.py
@@ -57,4 +57,6 @@
 
 frame = threads[0].GetFrameAtIndex(0)
 disassembly = frame.Disassemble()
-self.assertNotEqual(len(disassembly), 0, "Disassembly was empty.")
+self.assertNotEqual(disassembly, "")
+self.assertNotIn("error", disassembly)
+self.assertIn(": nop", disassembly)
Index: lldb/source/Target/StackFrame.cpp
===
--- lldb/source/Target/StackFrame.cpp
+++ lldb/source/Target/StackFrame.cpp
@@ -229,21 +229,16 @@
 
 const char *StackFrame::Disassemble() {
   std::lock_guard guard(m_mutex);
-  if (m_disassembly.Empty()) {
-ExecutionContext exe_ctx(shared_from_this());
-Target *target = exe_ctx.GetTargetPtr();
-if (target) {
-  const char *plugin_name = nullptr;
-  const char *flavor = nullptr;
-  Disassembler::Disassemble(target->GetDebugger(),
-target->GetArchitecture(), plugin_name, flavor,
-exe_ctx, 0, false, 0, 0, m_disassembly);
-}
-if (m_disassembly.Empty())
-  return nullptr;
+  if (!m_disassembly.Empty())
+return m_disassembly.GetData();
+
+  ExecutionContext exe_ctx(shared_from_this());
+  if (Target *target = exe_ctx.GetTargetPtr()) {
+Disassembler::Disassemble(target->GetDebugger(), target->GetArchitecture(),
+  *this, m_disassembly);
   }
 
-  return m_disassembly.GetData();
+  return m_disassembly.Empty() ? nullptr : m_disassembly.GetData();
 }
 
 Block *StackFrame::GetFrameBlock() {
Index: lldb/source/Core/Disassembler.cpp
===
--- lldb/source/Core/Disassembler.cpp
+++ lldb/source/Core/Disassembler.cpp
@@ -540,34 +540,29 @@
 }
 
 bool Disassembler::Disassemble(Debugger &debugger, const ArchSpec &arch,
-   const char *plugin_name, const char *flavor,
-   const ExecutionContext &exe_ctx,
-   uint32_t num_instructions,
-   bool mixed_source_and_assembly,
-   uint32_t num_mixed_context_lines,
-   uint32_t options, Stream &strm) {
+   StackFrame &frame, Stream &strm) {
   AddressRange range;
-  StackFrame *frame = exe_ctx.GetFramePtr();
-  if (frame) {
-SymbolContext sc(
-frame->GetSymbolContext(eSymbolContextFunction | eSymbolContextSymbol));
-if (sc.function) {
-  range = sc.function->GetAddressRange();
-} else if (sc.symbol && sc.symbol->ValueIsAddress()) {
-  range.GetBaseAddress() = sc.symbol->GetAddressRef();
-  range.SetByteSize(sc.symbol->GetByteSize());
-} else {
-  range.GetBaseAddress() = frame->GetFrameCodeAddress();
-}
+  SymbolContext sc(
+  frame.GetSymbolContext(eSymbolContextFunction | eSymbolContextSymbol));
+  if (sc.function) {
+range = sc.function->GetAddressRange();
+  } else if (sc.symbol && sc.symbol->ValueIsAddress()) {
+range.GetBaseAddress() = sc.symbol->GetAddressRef();
+range.SetByteSize(sc.symbol->GetByteSize());
+  } else {
+range.GetBaseAddress() = frame.GetFrameCodeAddress();
+  }
 
 if (range.GetBaseAddress().IsValid() && range.GetByteSize() == 0)
   range.SetByteSize(DEFAULT_DISASM_BYTE_SIZE);
-  }
 
-  return Disassemble(
-  debugger, arch, plugin_name, flavor, exe_ctx, range.GetBaseAddress(),
-  {Limit::Instructions, num_instructions}, mixed_source_and_assembly,
-  num_mixed_context_lines, options, strm);
+Disassembler::Limit limit = {Disassembler::Limit::Bytes

[Lldb-commits] [lldb] 826997c - [lldb] Fix a regression introduced by D75730

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-10-22T08:38:03-07:00
New Revision: 826997c46280351861be43522d4a022d8fdbc466

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

LOG: [lldb] Fix a regression introduced by D75730

In a new Range class was introduced to simplify and the Disassembler API
and reduce duplication. It unintentionally broke the
SBFrame::Disassemble functionality because it unconditionally converts
the number of instructions to a Range{Limit::Instructions,
num_instructions}. This is subtly different from the previous behavior,
where now we're passing a Range and assume it's valid in the callee, the
original code would propagate num_instructions and the callee would
compare the value and decided between disassembling instructions or
bytes.

Unfortunately the existing tests was not particularly strict:

  disassembly = frame.Disassemble()
  self.assertNotEqual(len(disassembly), 0, "Disassembly was empty.")

This would pass because without this patch we'd disassemble zero
instructions, resulting in an error:

  (lldb) script print(lldb.frame.Disassemble())
  error: error reading data from section __text

Differential revision: https://reviews.llvm.org/D89925

Added: 


Modified: 
lldb/include/lldb/Core/Disassembler.h
lldb/source/Core/Disassembler.cpp
lldb/source/Target/StackFrame.cpp
lldb/test/API/commands/disassemble/basic/TestFrameDisassemble.py
lldb/test/API/commands/disassemble/basic/main.cpp

Removed: 




diff  --git a/lldb/include/lldb/Core/Disassembler.h 
b/lldb/include/lldb/Core/Disassembler.h
index d3b903b83c19..c38f1f7975d5 100644
--- a/lldb/include/lldb/Core/Disassembler.h
+++ b/lldb/include/lldb/Core/Disassembler.h
@@ -48,6 +48,7 @@ class DataExtractor;
 class Debugger;
 class Disassembler;
 class Module;
+class StackFrame;
 class Stream;
 class SymbolContext;
 class SymbolContextList;
@@ -404,11 +405,8 @@ class Disassembler : public 
std::enable_shared_from_this,
   uint32_t num_mixed_context_lines, uint32_t options,
   Stream &strm);
 
-  static bool
-  Disassemble(Debugger &debugger, const ArchSpec &arch, const char 
*plugin_name,
-  const char *flavor, const ExecutionContext &exe_ctx,
-  uint32_t num_instructions, bool mixed_source_and_assembly,
-  uint32_t num_mixed_context_lines, uint32_t options, Stream 
&strm);
+  static bool Disassemble(Debugger &debugger, const ArchSpec &arch,
+  StackFrame &frame, Stream &strm);
 
   // Constructors and Destructors
   Disassembler(const ArchSpec &arch, const char *flavor);

diff  --git a/lldb/source/Core/Disassembler.cpp 
b/lldb/source/Core/Disassembler.cpp
index cc98b7309d6d..111cec6e2968 100644
--- a/lldb/source/Core/Disassembler.cpp
+++ b/lldb/source/Core/Disassembler.cpp
@@ -540,34 +540,29 @@ void Disassembler::PrintInstructions(Debugger &debugger, 
const ArchSpec &arch,
 }
 
 bool Disassembler::Disassemble(Debugger &debugger, const ArchSpec &arch,
-   const char *plugin_name, const char *flavor,
-   const ExecutionContext &exe_ctx,
-   uint32_t num_instructions,
-   bool mixed_source_and_assembly,
-   uint32_t num_mixed_context_lines,
-   uint32_t options, Stream &strm) {
+   StackFrame &frame, Stream &strm) {
   AddressRange range;
-  StackFrame *frame = exe_ctx.GetFramePtr();
-  if (frame) {
-SymbolContext sc(
-frame->GetSymbolContext(eSymbolContextFunction | 
eSymbolContextSymbol));
-if (sc.function) {
-  range = sc.function->GetAddressRange();
-} else if (sc.symbol && sc.symbol->ValueIsAddress()) {
-  range.GetBaseAddress() = sc.symbol->GetAddressRef();
-  range.SetByteSize(sc.symbol->GetByteSize());
-} else {
-  range.GetBaseAddress() = frame->GetFrameCodeAddress();
-}
+  SymbolContext sc(
+  frame.GetSymbolContext(eSymbolContextFunction | eSymbolContextSymbol));
+  if (sc.function) {
+range = sc.function->GetAddressRange();
+  } else if (sc.symbol && sc.symbol->ValueIsAddress()) {
+range.GetBaseAddress() = sc.symbol->GetAddressRef();
+range.SetByteSize(sc.symbol->GetByteSize());
+  } else {
+range.GetBaseAddress() = frame.GetFrameCodeAddress();
+  }
 
 if (range.GetBaseAddress().IsValid() && range.GetByteSize() == 0)
   range.SetByteSize(DEFAULT_DISASM_BYTE_SIZE);
-  }
 
-  return Disassemble(
-  debugger, arch, plugin_name, flavor, exe_ctx, range.GetBaseAddress(),
-  {Limit::Instructions, num_instructions}, mixed_source_and_assembly,
-  num_mixed_context_lines, options, strm);
+Disassembler::Limit limit

[Lldb-commits] [lldb] efe62b6 - [lldb/DWARF] Add support for DW_OP_implicit_value

[Med Ismail Bennani via lldb-commits]

Author: Med Ismail Bennani
Date: 2020-10-22T18:02:44+02:00
New Revision: efe62b637d51f6d622589132075320dd4f687478

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

LOG: [lldb/DWARF] Add support for DW_OP_implicit_value

This patch completes https://reviews.llvm.org/D83560. Now that the
compiler can emit `DW_OP_implicit_value` into DWARF expressions, lldb
needed to learn reading these opcodes for variable inspection and
expression evaluation.

This implicit location descriptor specifies an immediate value with two
operands: the length (ULEB128) followed by a block representing the value
in the target memory representation.

rdar://67406091

Differential revision: https://reviews.llvm.org/D89842

Signed-off-by: Med Ismail Bennani 

Added: 


Modified: 
lldb/source/Expression/DWARFExpression.cpp
lldb/unittests/Expression/DWARFExpressionTest.cpp

Removed: 




diff  --git a/lldb/source/Expression/DWARFExpression.cpp 
b/lldb/source/Expression/DWARFExpression.cpp
index 87e2c8243939..b42c9cfafb4f 100644
--- a/lldb/source/Expression/DWARFExpression.cpp
+++ b/lldb/source/Expression/DWARFExpression.cpp
@@ -2248,6 +2248,29 @@ bool DWARFExpression::Evaluate(
   }
   break;
 
+// OPCODE: DW_OP_implicit_value
+// OPERANDS: 2
+//  ULEB128  size of the value block in bytes
+//  uint8_t* block bytes encoding value in target's memory
+//  representation
+// DESCRIPTION: Value is immediately stored in block in the debug info with
+// the memory representation of the target.
+case DW_OP_implicit_value: {
+  const uint32_t len = opcodes.GetULEB128(&offset);
+  const void *data = opcodes.GetData(&offset, len);
+
+  if (!data) {
+LLDB_LOG(log, "Evaluate_DW_OP_implicit_value: could not be read data");
+LLDB_ERRORF(error_ptr, "Could not evaluate %s.",
+DW_OP_value_to_name(op));
+return false;
+  }
+
+  Value result(data, len);
+  stack.push_back(result);
+  break;
+}
+
 // OPCODE: DW_OP_push_object_address
 // OPERANDS: none
 // DESCRIPTION: Pushes the address of the object currently being

diff  --git a/lldb/unittests/Expression/DWARFExpressionTest.cpp 
b/lldb/unittests/Expression/DWARFExpressionTest.cpp
index c02de402b4dd..1c762677f6fe 100644
--- a/lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ b/lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -218,6 +218,14 @@ TEST(DWARFExpression, DW_OP_piece) {
   llvm::HasValue(GetScalar(16, 0xff00, true)));
 }
 
+TEST(DWARFExpression, DW_OP_implicit_value) {
+  unsigned char bytes = 4;
+
+  EXPECT_THAT_EXPECTED(
+  Evaluate({DW_OP_implicit_value, bytes, 0x11, 0x22, 0x33, 0x44}),
+  llvm::HasValue(GetScalar(8 * bytes, 0x44332211, true)));
+}
+
 TEST(DWARFExpression, DW_OP_unknown) {
   EXPECT_THAT_EXPECTED(
   Evaluate({0xff}),



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

[Lldb-commits] [lldb] 5dc7033 - Revert "[lldb] Explicitly use the configuration architecture when building test executables"

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2020-10-22T18:42:19+02:00
New Revision: 5dc70332d53cc5744aedf72a12d0367988559776

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

LOG: Revert "[lldb] Explicitly use the configuration architecture when building 
test executables"

This reverts commit 41185226f6d80663b4a1064c6f47581ee567d78d.

Causes TestQuoting to fail on Windows.

Added: 


Modified: 
lldb/packages/Python/lldbsuite/test/builders/builder.py
lldb/packages/Python/lldbsuite/test/lldbtest.py

Removed: 




diff  --git a/lldb/packages/Python/lldbsuite/test/builders/builder.py 
b/lldb/packages/Python/lldbsuite/test/builders/builder.py
index 6c9584224f4a..fbfa86700e22 100644
--- a/lldb/packages/Python/lldbsuite/test/builders/builder.py
+++ b/lldb/packages/Python/lldbsuite/test/builders/builder.py
@@ -93,7 +93,11 @@ def getArchSpec(self, architecture):
 Helper function to return the key-value string to specify the 
architecture
 used for the make system.
 """
-return ("ARCH=" + architecture) if architecture else ""
+arch = architecture if architecture else None
+if not arch and configuration.arch:
+arch = configuration.arch
+
+return ("ARCH=" + arch) if arch else ""
 
 def getCCSpec(self, compiler):
 """

diff  --git a/lldb/packages/Python/lldbsuite/test/lldbtest.py 
b/lldb/packages/Python/lldbsuite/test/lldbtest.py
index f469ce86f26e..69da3914f1f2 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -2606,9 +2606,6 @@ def build(
 """Platform specific way to build the default binaries."""
 module = builder_module()
 
-if not architecture and configuration.arch:
-architecture = configuration.arch
-
 dictionary = lldbplatformutil.finalize_build_dictionary(dictionary)
 if self.getDebugInfo() is None:
 return self.buildDefault(architecture, compiler, dictionary)



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

[Lldb-commits] [PATCH] D89600: [lldb] Move copying of reproducer out of process

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 300051.
JDevlieghere marked 6 inline comments as done.
JDevlieghere added a comment.

Address Pavel's code review feedback


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

https://reviews.llvm.org/D89600

Files:
  lldb/include/lldb/API/SBReproducer.h
  lldb/include/lldb/Host/FileSystem.h
  lldb/include/lldb/Utility/Reproducer.h
  lldb/include/lldb/Utility/ReproducerProvider.h
  lldb/source/API/SBReproducer.cpp
  lldb/source/Commands/CommandObjectReproducer.cpp
  lldb/source/Host/common/FileSystem.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ModuleDependencyCollector.h
  lldb/source/Utility/Reproducer.cpp
  lldb/source/Utility/ReproducerProvider.cpp
  lldb/test/Shell/Reproducer/TestCaptureEnvOverride.test
  lldb/test/Shell/Reproducer/TestFinalize.test
  lldb/tools/driver/Driver.cpp
  lldb/tools/driver/Options.td

Index: lldb/tools/driver/Options.td
===
--- lldb/tools/driver/Options.td
+++ lldb/tools/driver/Options.td
@@ -232,6 +232,8 @@
 def replay: Separate<["--", "-"], "replay">,
   MetaVarName<"">,
   HelpText<"Tells the debugger to replay a reproducer from .">;
+def reproducer_finalize: Separate<["--", "-"], "reproducer-finalize">,
+  MetaVarName<"">;
 def no_version_check: F<"reproducer-no-version-check">,
   HelpText<"Disable the reproducer version check.">;
 def no_verification: F<"reproducer-no-verify">,
Index: lldb/tools/driver/Driver.cpp
===
--- lldb/tools/driver/Driver.cpp
+++ lldb/tools/driver/Driver.cpp
@@ -729,25 +729,10 @@
   signal(signo, sigcont_handler);
 }
 
-void reproducer_handler(void *argv0) {
+void reproducer_handler(void *finalize_cmd) {
   if (SBReproducer::Generate()) {
-auto exe = static_cast(argv0);
-llvm::outs() << "\n";
-llvm::outs() << "Crash reproducer for ";
-llvm::outs() << lldb::SBDebugger::GetVersionString() << '\n';
-llvm::outs() << '\n';
-llvm::outs() << "Reproducer written to '" << SBReproducer::GetPath()
- << "'\n";
-llvm::outs() << '\n';
-llvm::outs() << "Before attaching the reproducer to a bug report:\n";
-llvm::outs() << " - Look at the directory to ensure you're willing to "
-"share its content.\n";
-llvm::outs()
-<< " - Make sure the reproducer works by replaying the reproducer.\n";
-llvm::outs() << '\n';
-llvm::outs() << "Replay the reproducer with the following command:\n";
-llvm::outs() << exe << " -replay " << SBReproducer::GetPath() << "\n";
-llvm::outs() << "\n";
+std::system(static_cast(finalize_cmd));
+fflush(stdout);
   }
 }
 
@@ -799,6 +784,31 @@
 
 llvm::Optional InitializeReproducer(llvm::StringRef argv0,
  opt::InputArgList &input_args) {
+  if (auto *finalize_path = input_args.getLastArg(OPT_reproducer_finalize)) {
+if (const char *error = SBReproducer::Finalize(finalize_path->getValue())) {
+  WithColor::error() << "reproducer finalization failed: " << error << '\n';
+  return 1;
+}
+
+llvm::outs() << "\n";
+llvm::outs() << "Crash reproducer for ";
+llvm::outs() << lldb::SBDebugger::GetVersionString() << '\n';
+llvm::outs() << '\n';
+llvm::outs() << "Reproducer written to '" << SBReproducer::GetPath()
+ << "'\n";
+llvm::outs() << '\n';
+llvm::outs() << "Before attaching the reproducer to a bug report:\n";
+llvm::outs() << " - Look at the directory to ensure you're willing to "
+"share its content.\n";
+llvm::outs()
+<< " - Make sure the reproducer works by replaying the reproducer.\n";
+llvm::outs() << '\n';
+llvm::outs() << "Replay the reproducer with the following command:\n";
+llvm::outs() << argv0 << " -replay " << finalize_path->getValue() << "\n";
+llvm::outs() << "\n";
+return 0;
+  }
+
   if (auto *replay_path = input_args.getLastArg(OPT_replay)) {
 SBReplayOptions replay_options;
 replay_options.SetCheckVersion(!input_args.hasArg(OPT_no_version_check));
@@ -821,12 +831,6 @@
   }
 
   if (capture || capture_path) {
-// Register the reproducer signal handler.
-if (!input_args.hasArg(OPT_no_generate_on_signal)) {
-  llvm::sys::AddSignalHandler(reproducer_handler,
-  const_cast(argv0.data()));
-}
-
 if (capture_path) {
   if (!capture)
 WithColor::warning() << "-capture-path specified without -capture\n";
@@ -843,6 +847,19 @@
 }
 if (generate_on_exit)
   SBReproducer::SetAutoGenerate(true);
+
+// Register the reproducer signal handler.
+if (!input_args.hasArg(OPT_no_generate_on_signal)) {
+  if (const char *reproducer_path = SBReproducer::GetPath()) {
+// Leaking the string on purpose.
+std::string *fina

[Lldb-commits] [PATCH] D87172: Check if debug line sequences are starting after the first code segment

[António Afonso via Phabricator via lldb-commits]

aadsm added inline comments.



Comment at: 
lldb/test/Shell/SymbolFile/DWARF/line-entries-invalid-addresses.yaml:3
+# RUN: %lldb %t -b -o "image lookup -f main.cpp -l 2 -v" | FileCheck %s
+# CHECK: LineEntry: {{.*}}main.cpp:2:{{.*}}
+

labath wrote:
> I think you also need to check for the "1 matches found", otherwise this will 
> succeed even without the patch.
This should be fine, this is what you get without the patch:

```
(lldb) image lookup -f main.cpp -l 2 -v
1 match found in main.cpp:2 in /Users/aadsm/Projects/test-bad.obj:
Address:  ()
Summary:
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87172

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

[Lldb-commits] [PATCH] D87172: Check if debug line sequences are starting after the first code segment

[António Afonso via Phabricator via lldb-commits]

aadsm updated this revision to Diff 300131.
aadsm added a comment.

Check all child sections and makes sure the section is an actual code section.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87172

Files:
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/test/Shell/SymbolFile/DWARF/line-entries-invalid-addresses.yaml

Index: lldb/test/Shell/SymbolFile/DWARF/line-entries-invalid-addresses.yaml
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/line-entries-invalid-addresses.yaml
@@ -0,0 +1,420 @@
+# RUN: yaml2obj %s > %t
+# RUN: %lldb %t -b -o "image lookup -f main.cpp -l 2 -v" | FileCheck %s
+# CHECK: LineEntry: {{.*}}main.cpp:2:{{.*}}
+
+# int foo() {
+# return 42;
+# }
+#
+# int main() {
+# int x = foo();
+# return x;
+# }
+# dwarfdump -debug-line:
+# AddressLine   Column File   ISA Discriminator Flags
+# -- -- -- -- --- - -
+# 0x00010f80  1  0  1   0 0  is_stmt
+# 0x00010f84  2  5  1   0 0  is_stmt prologue_end
+# 0x00010f8b  2  5  1   0 0  is_stmt end_sequence
+# 0x00010f90  5  0  1   0 0  is_stmt
+# 0x00010f90  5  0  1   0 0  is_stmt end_sequence
+# 0x000f  2 13  1   0 0  is_stmt prologue_end
+# 0x0014  2  9  1   0 0
+# 0x0017  3 12  1   0 0  is_stmt
+# 0x001d  3 12  1   0 0  end_sequence
+--- !mach-o
+FileHeader:
+  magic:   0xFEEDFACF
+  cputype: 0x0107
+  cpusubtype:  0x0003
+  filetype:0x000A
+  ncmds:   7
+  sizeofcmds:  1400
+  flags:   0x
+  reserved:0x
+LoadCommands:
+  - cmd: LC_UUID
+cmdsize: 24
+uuid:605D6BBF-4DB2-39F7-8068-F9BB3896DF0C
+  - cmd: LC_BUILD_VERSION
+cmdsize: 24
+platform:1
+minos:   659200
+sdk: 659206
+ntools:  0
+  - cmd: LC_SYMTAB
+cmdsize: 24
+symoff:  4096
+nsyms:   3
+stroff:  4144
+strsize: 37
+  - cmd: LC_SEGMENT_64
+cmdsize: 72
+segname: __PAGEZERO
+vmaddr:  0
+vmsize:  1
+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
+maxprot: 5
+initprot:5
+nsects:  2
+flags:   0
+Sections:
+  - sectname:__text
+segname: __TEXT
+addr:0x00010F80
+size:48
+offset:  0x
+align:   4
+reloff:  0x
+nreloc:  0
+flags:   0x8400
+reserved1:   0x
+reserved2:   0x
+reserved3:   0x
+content: CFFAEDFE070103000A00070078051B001800605D6BBF4DB239F7
+  - sectname:__unwind_info
+segname: __TEXT
+addr:0x00010FB0
+size:72
+offset:  0x
+align:   2
+reloff:  0x
+nreloc:  0
+flags:   0x
+reserved1:   0x
+reserved2:   0x
+reserved3:   0x
+content: CFFAEDFE070103000A00070078051B001800605D6BBF4DB239F78068F9BB3896DF0C32001800010F0A00
+  - cmd: LC_SEGMENT_64
+cmdsize: 72
+segname: __LINKEDIT
+vmaddr:  4294971392
+vmsize:  4096
+fileoff: 4096
+filesize:85
+maxprot: 1
+initprot:1
+nsects:  0
+flags:   0
+  - cmd: LC_SEGMENT_64
+cmdsize: 952
+segname: __DWARF
+vmaddr:  4294975488
+vmsize:  4096
+fileoff: 8192
+filesize:839
+maxprot: 7
+initprot:3
+nsects:  4
+flags:   0
+Sections:
+  - sectname:__debug_line
+segname: __DWARF
+addr:0x00

[Lldb-commits] [PATCH] D89812: [lldb][PDB] Add ObjectFile PDB plugin

[Zequan Wu via Phabricator via lldb-commits]

zequanwu added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:176-197
+  switch (dbi_stream->getMachineType()) {
+  case PDB_Machine::Amd64:
+spec.SetTriple("x86_64-pc-windows");
+specs.Append(module_spec);
+break;
+  case PDB_Machine::x86:
+spec.SetTriple("i386-pc-windows");

labath wrote:
> zequanwu wrote:
> > labath wrote:
> > > Could this use the same algorithm as `ObjectFilePDB::GetArchitecture` ?
> > No. This part is the same algorithm as the part in 
> > `ObjectFilePECOFF::GetModuleSpecifications`, 
> > https://github.com/llvm/llvm-project/blob/master/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp#L194-L215.
> > 
> > The `ObjectFilePDB::GetArchitecture` is same as 
> > `ObjectFilePECOFF::GetArchitecture`.
> I'm not sure this kind of consistency actually helps. For example, in 
> `Module::MatchesModuleSpec`, module's own architecture (as provided by 
> `ObjectFile(PECOFF)::GetArchitecture`) is compared to the one in the 
> ModuleSpec (generally coming from `ObjectFile(PDB)::GetModuleSpecifications`.
> 
> If there's any mismatch between the two ways of computing a ArchSpec, then 
> there will be trouble, even though they are "symmetric".
> 
> That said, I think we can keep this like it is in this patch, but I'd like to 
> clean up both implementations in a another patch.
So, we want to refactor out the common part of 
`ObjectFilePECOFF::GetArchitecture` and 
`ObjectFilePDB::GetModuleSpecifications` to keep them consistent? I will do it 
in another patch.




Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:207-209
+Address ObjectFilePDB::GetBaseAddress() {
+  return Address(GetSectionList()->GetSectionAtIndex(0), 0);
+}

labath wrote:
> zequanwu wrote:
> > labath wrote:
> > > Leave this unimplemented. PDBs don't have sections and are not loaded 
> > > into memory, so the function does not apply.
> > This is actually necessary. It's called at 
> > `SymbolFileNativePDB::InitializeObject`. Otherwise, setting breakpoint 
> > won't work.
> So, that's a bug in `SymbolFileNativePDB::InitializeObject`. It should be 
> calling `m_objfile_sp->GetModule()->GetObjectFile()->GetBaseAddress()` (as 
> done in e.g. `SymbolFileBreakpad::GetBaseFileAddress`) instead of 
> `m_objfile_sp->GetBaseAddress()`. That will get you the base address of the 
> main (exe) object file instead of the pdb. Up until now, that distinction was 
> not important because `m_objfile_sp` was always the main file...
Thanks for pointing it out. I'll update it and leave `GetBaseAddress` and 
`CreateSections` as unimplemented.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:299
 std::unique_ptr file_up =
 loadMatchingPDBFile(m_objfile_sp->GetFileSpec().GetPath(), 
m_allocator);
 

labath wrote:
> zequanwu wrote:
> > labath wrote:
> > > Instead of changing `loadMatchingPDBFile` I think we ought to change this 
> > > to something like:
> > > ```
> > > if (auto *pdb = llvm::dyn_cast(m_objfile_sp)) {
> > >   file_up = pdb->giveMeAPDBFile(); 
> > >   // PS: giveMeAPDBFile is a new pdb-specific public method on 
> > > ObjectFilePDB
> > >   // PS: possibly this should not return a PDBFile, but a PDBIndex 
> > > directly -- I don't know the details but the general idea is that it 
> > > could/should reuse the pdb parsing state that the objectfile class has 
> > > already created
> > > } else 
> > >   file_up = do_the_loadMatchingPDBFile_fallback();
> > > ```
> > > 
> > > The reason for that is that it avoids opening the pdb file twice (once in 
> > > the object file and once here). Another reason is that I believe the 
> > > entire `loadMatchingPDBFile` function should disappear. Finding the pdb 
> > > file should not be the job of the symbol file class -- we have symbol 
> > > vendors for that. However, we should keep the fallback for now to keep 
> > > the patch small...
> > From what I see, it seems like PDBFile is designed to be created when 
> > opening the pdb file. It doesn't allow copying it around.
> > `NativeSession` holds a unique_ptr of PDBFile and 
> > `NativeSession::getPDBFile` returns the reference of object, so creating 
> > unique_ptr from it is not allowed.
> > 
> > I moved `loadPDBFile` to `ObjectFilePDB` as static function, so we can move 
> > `PDBFile` easily.
> You don't need to copy it -- you can just have the two objects reference the 
> same instance (stealing the PDBFile object from under ObjectFilePDB seems 
> suboptimal). Probably the simplest way to achieve that is to change the 
> PdbIndex class to use a raw pointer (and ObjectFilePDB to provide one). Then 
> SymbolFilePDB can have an additional unique_ptr member, which may or 
> may not be empty (depending on whether it fetches object from ObjectFilePDB 
> or loads it itself). Eventually, as we transition to always fetching the 
> PDBFile from ObjectFile

[Lldb-commits] [PATCH] D89812: [lldb][PDB] Add ObjectFile PDB plugin

[Zequan Wu via Phabricator via lldb-commits]

zequanwu updated this revision to Diff 300133.
zequanwu marked 4 inline comments as done.
zequanwu added a comment.
Herald added a subscriber: arphaman.

Address some comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89812

Files:
  lldb/source/Plugins/ObjectFile/CMakeLists.txt
  lldb/source/Plugins/ObjectFile/PDB/CMakeLists.txt
  lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
  lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.h
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/CMakeLists.txt
  lldb/source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/PdbIndex.h
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/test/Shell/ObjectFile/PDB/Inputs/pdb.yaml
  lldb/test/Shell/ObjectFile/PDB/object.test
  lldb/test/Shell/ObjectFile/PDB/symbol.test
  lldb/test/Shell/SymbolFile/NativePDB/load-pdb.cpp

Index: lldb/test/Shell/SymbolFile/NativePDB/load-pdb.cpp
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/NativePDB/load-pdb.cpp
@@ -0,0 +1,22 @@
+// clang-format off
+// REQUIRES: lld, x86
+
+// Test that lldb load PDB file by command `target symbols add`
+
+// RUN: mkdir -p %t/executable
+// RUN: rm -f %t/executable/foo.exe %t/executable/bar.pdb
+// RUN: %clang_cl --target=x86_64-windows-msvc -Od -Z7 -c /Fo%t/executable/foo.obj -- %s
+// RUN: lld-link -debug:full -nodefaultlib -entry:main %t/executable/foo.obj \
+// RUN: -out:%t/executable/foo.exe -pdb:%t/executable/foo.pdb
+// Rename the PDB file so that the name is different from the name inside the executable (foo.exe).
+// RUN: mv %t/executable/foo.pdb %t/executable/bar.pdb
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -O \
+// RUN: "target create %t/executable/foo.exe" -o \
+// RUN: "target symbols add %t/executable/bar.pdb" -o "quit" | FileCheck %s
+
+int main(int argc, char** argv) {
+  return 0;
+}
+
+// CHECK: (lldb) target symbols add {{.*}}bar.pdb
+// CHECK: symbol file '{{.*}}/bar.pdb' has been added to '{{.*}}/foo.exe'
Index: lldb/test/Shell/ObjectFile/PDB/symbol.test
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/PDB/symbol.test
@@ -0,0 +1,10 @@
+# REQUIRES: lld
+# RUN: yaml2obj %p/Inputs/pdb.yaml -o %t.obj
+# RUN: lld-link %t.obj /debug /pdb:%t.pdb /entry:main /nodefaultlib
+# RUN: lldb-test symbols %t.pdb | FileCheck %s
+
+# CHECK: Types:
+# CHECK-NEXT: {{.*}}:   Type{0x00010024} , size = 0, compiler_type = {{.*}} int (int, char **)
+# CHECK: Compile units:
+# CHECK-NEXT: {{.*}}:   CompileUnit{0x}, language = "c++", file = '/tmp/a.cpp'
+# CHECK-NEXT: {{.*}}: Function{0x0841}, demangled = main, type = {{.*}}
\ No newline at end of file
Index: lldb/test/Shell/ObjectFile/PDB/object.test
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/PDB/object.test
@@ -0,0 +1,14 @@
+# REQUIRES: lld
+# RUN: yaml2obj %p/Inputs/pdb.yaml -o %t.obj
+# RUN: lld-link %t.obj /debug /pdb:%t.pdb /entry:main /nodefaultlib
+# RUN: lldb-test object-file %t.pdb | FileCheck %s
+
+# CHECK: Plugin name: pdb
+# CHECK: Architecture: x86_64-pc-windows-msvc
+# CHECK: UUID: AB70D9A0-FB42-B92C-4C4C-44205044422E-0001
+# CHECK: Executable: false
+# CHECK: Stripped: false
+# CHECK: Type: debug info
+# CHECK: Strata: user
+# CHECK: Base VM address: 0x
+# CHECK: There are no sections
Index: lldb/test/Shell/ObjectFile/PDB/Inputs/pdb.yaml
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/PDB/Inputs/pdb.yaml
@@ -0,0 +1,314 @@
+--- !COFF
+header:
+  Machine: IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:
+  - Name:.text
+Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+Alignment:   16
+SectionData: 4883EC1831C0C74424144889542408894C24044883C418C3
+  - Name:.data
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+Alignment:   4
+SectionData: ''
+  - Name:.bss
+Characteristics: [ IMAGE_SCN_CNT_UNINITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+Alignment:   4
+SectionData: ''
+SizeOfRawData:   0
+  - Name:'.debug$S'
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_DISCARDABLE, IMAGE_SCN_MEM_READ ]
+Alignment:   4
+SectionData: 0400F10080007E003C110100DC00E02E636C616E672076657273696F6E2031322E302E302028676974406769746875622E636F6D3A6C6C766D2F6C6C766D2D70726F6A6563742E676974203861303865303864623663326534613564623438353235336633313836623066396537333965313529F10090002A004711000

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 300134.
wallace added a comment.

The diff is the now ready for review. There are a few updates, including some 
design decisions after some chats with Greg.

- Now the dump command includes disassembly information and symbol context 
information whenever relevant, e.g.

   a.out`main + 15 at main.cpp:10
[ 0] 0x0040066fcallq  0x400540  ; symbol stub 
for: foo()
  a.out`symbol stub for: foo()
[ 1] 0x00400540jmpq   *0x200ae2(%rip)   ; 
_GLOBAL_OFFSET_TABLE_ + 40
[ 2] 0x00400546pushq  $0x2
[ 3] 0x0040054bjmp0x400510
  a.out`(none)
[ 4] 0x00400510pushq  0x200af2(%rip); 
_GLOBAL_OFFSET_TABLE_ + 8
[ 5] 0x00400516jmpq   *0x200af4(%rip)   ; 
_GLOBAL_OFFSET_TABLE_ + 16
[ 6] 0x77df1950error: no memory mapped at this address
...instructions missing
  a.out`main + 20 at main.cpp:10
[ 7] 0x00400674movl   %eax, -0xc(%rbp)
  a.out`main + 23 at main.cpp:12
[ 8] 0x00400677movl   -0xc(%rbp), %eax



- A flag for the command has been added (-r), which prints raw instruction 
addresses, similar to the one in the disassembly command, e.g.

  ``` > thread trace dump instructions --raw

  thread #1: tid = 3842849, total instructions = 21 [ 1] 0x00400518 [ 
2] 0x0040051f [ 3] 0x00400529 [ 4] 0x0040052d [ 5] 
0x00400521 [ 6] 0x00400525

  - I tried to use yaml to represent the binary fails, but it doesn't work. The 
yaml representation doesn't have all the required information and the decoder 
complaints about missing memory. At this point it's better to just include the 
binary and have strong tests.
  
  - I'm leaving for a future diff restructuring the IntelPTInstruction class so 
that is more memory efficient. Something to keep in mind is that errors will be 
seldom, as they represent gaps in the trace, and most of the instructions will 
be correct addresses.


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

https://reviews.llvm.org/D89283

Files:
  lldb/include/lldb/Core/Disassembler.h
  lldb/include/lldb/Symbol/SymbolContext.h
  lldb/include/lldb/Target/Trace.h
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  lldb/source/Core/Disassembler.cpp
  lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
  lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Symbol/SymbolContext.cpp
  lldb/source/Target/ProcessTrace.cpp
  lldb/source/Target/Trace.cpp
  lldb/source/Target/TraceSessionFileParser.cpp
  lldb/test/API/commands/trace/TestTraceDumpInstructions.py
  lldb/test/API/commands/trace/intelpt-trace-multi-file/a.out
  lldb/test/API/commands/trace/intelpt-trace-multi-file/bar.cpp
  lldb/test/API/commands/trace/intelpt-trace-multi-file/bar.h
  lldb/test/API/commands/trace/intelpt-trace-multi-file/foo.cpp
  lldb/test/API/commands/trace/intelpt-trace-multi-file/foo.h
  lldb/test/API/commands/trace/intelpt-trace-multi-file/libbar.so
  lldb/test/API/commands/trace/intelpt-trace-multi-file/libfoo.so
  lldb/test/API/commands/trace/intelpt-trace-multi-file/main.cpp
  lldb/test/API/commands/trace/intelpt-trace-multi-file/multi-file-no-ld.json
  lldb/test/API/commands/trace/intelpt-trace-multi-file/multi-file.trace
  lldb/test/API/commands/trace/intelpt-trace/trace_bad_image.json
  lldb/test/API/commands/trace/intelpt-trace/trace_wrong_cpu.json

Index: lldb/test/API/commands/trace/intelpt-trace/trace_wrong_cpu.json
===
--- /dev/null
+++ lldb/test/API/commands/trace/intelpt-trace/trace_wrong_cpu.json
@@ -0,0 +1,31 @@
+{
+  "trace": {
+"type": "intel-pt",
+"pt_cpu": {
+  "vendor": "intel",
+  "family": 2123123,
+  "model": 12123123,
+  "stepping": 1231231
+}
+  },
+  "processes": [
+{
+  "pid": 1234,
+  "triple": "x86_64-*-linux",
+  "threads": [
+{
+  "tid": 3842849,
+  "traceFile": "3842849.trace"
+}
+  ],
+  "modules": [
+{
+  "file": "a.out",
+  "systemPath": "a.out",
+  "loadAddress": "0x0040",
+  "uuid": "6AA9A4E2-6F28-2F33-377D-59FECE874C71-5B41261A"
+}
+  ]
+}
+  ]
+}
Index: lldb/test/API/commands/trace/intelpt-trace/trace_bad_image.json
===
--- /dev/null
+++ lldb/test/API/commands/trace/intelpt-trace/trace_bad_image.json
@@ -0,0 +1,31 @@
+{
+  "trace": {
+"type": "intel-pt",
+"pt_cpu": {
+  "vendor

[Lldb-commits] [PATCH] D89999: [lldb] Implement ObjCGetClassNameRaw using a UtilityFunction

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added a reviewer: jingham.
JDevlieghere requested review of this revision.

The static function ObjCGetClassNameRaw in AppleObjCRuntimeV2.cpp gets called 
fairly frequently to resolve the names of classes.  It runs a UserExpression, 
rather than making a UtilityFunction which means it is compiling, and JITing 
the expression every time it is called.  For something that’s called often like 
this within lldb it’s more efficient to write a UtilityFunction wrapper so you 
only have to JIT once.


https://reviews.llvm.org/D8

Files:
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
  lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h

Index: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h
===
--- lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h
+++ lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h
@@ -322,6 +322,9 @@
 
   bool GetCFBooleanValuesIfNeeded();
 
+  llvm::Expected
+  ObjCGetClassNameRaw(AppleObjCRuntime::ObjCISA isa);
+
   friend class ClassDescriptorV2;
 
   std::unique_ptr m_get_class_info_code;
@@ -332,6 +335,10 @@
   lldb::addr_t m_get_shared_cache_class_info_args;
   std::mutex m_get_shared_cache_class_info_args_mutex;
 
+  std::unique_ptr m_debug_class_get_name_raw;
+  lldb::addr_t m_debug_class_get_name_raw_args;
+  std::mutex m_debug_class_get_name_raw_args_mutex;
+
   std::unique_ptr m_decl_vendor_up;
   lldb::addr_t m_tagged_pointer_obfuscator;
   lldb::addr_t m_isa_hash_table_ptr;
Index: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
===
--- lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
+++ lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
@@ -73,6 +73,19 @@
 
 char AppleObjCRuntimeV2::ID = 0;
 
+static const char *g_debug_class_get_name_raw_name =
+"__lldb_apple_objc_v2_debug_class_getNameRaw";
+
+static const char *g_debug_class_get_name_raw_body = R"(
+extern "C"
+{
+  const char *objc_debug_class_getNameRaw(Class cls);
+}
+const char* __lldb_apple_objc_v2_debug_class_getNameRaw(void* cls) {
+  return objc_debug_class_getNameRaw((Class)cls);
+}
+)";
+
 static const char *g_get_dynamic_class_info_name =
 "__lldb_apple_objc_v2_get_dynamic_class_info";
 // Testing using the new C++11 raw string literals. If this breaks GCC then we
@@ -1175,26 +1188,119 @@
   return class_descriptor_sp;
 }
 
-static std::pair ObjCGetClassNameRaw(
-  AppleObjCRuntime::ObjCISA isa,
-  Process *process) {
-  StreamString expr_string;
-  std::string input = std::to_string(isa);
-  expr_string.Printf("(const char *)objc_debug_class_getNameRaw(%s)",
- input.c_str());
-
-  ValueObjectSP result_sp;
-  EvaluateExpressionOptions eval_options;
-  eval_options.SetLanguage(lldb::eLanguageTypeObjC);
-  eval_options.SetResultIsInternal(true);
-  eval_options.SetGenerateDebugInfo(true);
-  eval_options.SetTimeout(process->GetUtilityExpressionTimeout());
-  auto eval_result = process->GetTarget().EvaluateExpression(
-  expr_string.GetData(),
-  process->GetThreadList().GetSelectedThread()->GetSelectedFrame().get(),
-  result_sp, eval_options);
-  ConstString type_name(result_sp->GetSummaryAsCString());
-  return std::make_pair(eval_result == eExpressionCompleted, type_name);
+llvm::Expected
+AppleObjCRuntimeV2::ObjCGetClassNameRaw(AppleObjCRuntime::ObjCISA isa) {
+  Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS | LIBLLDB_LOG_TYPES));
+
+  ThreadSP thread_sp =
+  m_process->GetThreadList().GetExpressionExecutionThread();
+  if (!thread_sp)
+return llvm::make_error(
+"could not get the current thread", llvm::inconvertibleErrorCode());
+
+  ExecutionContext exe_ctx;
+  thread_sp->CalculateExecutionContext(exe_ctx);
+
+  TypeSystemClang *ast = TypeSystemClang::GetScratch(m_process->GetTarget());
+  if (!ast)
+return llvm::make_error(
+"could not get clang type system", llvm::inconvertibleErrorCode());
+
+  CompilerType clang_void_pointer_type =
+  ast->GetBasicType(eBasicTypeVoid).GetPointerType();
+  CompilerType clang_char_pointer_type =
+  ast->GetBasicType(eBasicTypeChar).GetPointerType();
+
+  FunctionCaller *debug_class_get_name_raw_function = nullptr;
+  Status error;
+  ValueList arguments;
+  DiagnosticManager diagnostics;
+
+  if (!m_debug_class_get_name_raw) {
+m_debug_class_get_name_raw.reset(
+GetTargetRef().GetUtilityFunctionForLanguage(
+g_debug_class_get_name_raw_body, eLanguageTypeObjC,
+g_debug_class_get_name_raw_name, error));
+
+if (error.Fail())
+  return error.ToError();
+
+if (!m

[Lldb-commits] [PATCH] D89999: [lldb] Implement ObjCGetClassNameRaw using a UtilityFunction

[Jonas Devlieghere via Phabricator via lldb-commits]

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

- Remove accidentally staged changes


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

https://reviews.llvm.org/D8

Files:
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
  lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h

Index: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h
===
--- lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h
+++ lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h
@@ -322,6 +322,9 @@
 
   bool GetCFBooleanValuesIfNeeded();
 
+  llvm::Expected
+  ObjCGetClassNameRaw(AppleObjCRuntime::ObjCISA isa);
+
   friend class ClassDescriptorV2;
 
   std::unique_ptr m_get_class_info_code;
@@ -332,6 +335,8 @@
   lldb::addr_t m_get_shared_cache_class_info_args;
   std::mutex m_get_shared_cache_class_info_args_mutex;
 
+  std::unique_ptr m_debug_class_get_name_raw;
+
   std::unique_ptr m_decl_vendor_up;
   lldb::addr_t m_tagged_pointer_obfuscator;
   lldb::addr_t m_isa_hash_table_ptr;
Index: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
===
--- lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
+++ lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
@@ -73,6 +73,19 @@
 
 char AppleObjCRuntimeV2::ID = 0;
 
+static const char *g_debug_class_get_name_raw_name =
+"__lldb_apple_objc_v2_debug_class_getNameRaw";
+
+static const char *g_debug_class_get_name_raw_body = R"(
+extern "C"
+{
+  const char *objc_debug_class_getNameRaw(Class cls);
+}
+const char* __lldb_apple_objc_v2_debug_class_getNameRaw(void* cls) {
+  return objc_debug_class_getNameRaw((Class)cls);
+}
+)";
+
 static const char *g_get_dynamic_class_info_name =
 "__lldb_apple_objc_v2_get_dynamic_class_info";
 // Testing using the new C++11 raw string literals. If this breaks GCC then we
@@ -1175,26 +1188,119 @@
   return class_descriptor_sp;
 }
 
-static std::pair ObjCGetClassNameRaw(
-  AppleObjCRuntime::ObjCISA isa,
-  Process *process) {
-  StreamString expr_string;
-  std::string input = std::to_string(isa);
-  expr_string.Printf("(const char *)objc_debug_class_getNameRaw(%s)",
- input.c_str());
-
-  ValueObjectSP result_sp;
-  EvaluateExpressionOptions eval_options;
-  eval_options.SetLanguage(lldb::eLanguageTypeObjC);
-  eval_options.SetResultIsInternal(true);
-  eval_options.SetGenerateDebugInfo(true);
-  eval_options.SetTimeout(process->GetUtilityExpressionTimeout());
-  auto eval_result = process->GetTarget().EvaluateExpression(
-  expr_string.GetData(),
-  process->GetThreadList().GetSelectedThread()->GetSelectedFrame().get(),
-  result_sp, eval_options);
-  ConstString type_name(result_sp->GetSummaryAsCString());
-  return std::make_pair(eval_result == eExpressionCompleted, type_name);
+llvm::Expected
+AppleObjCRuntimeV2::ObjCGetClassNameRaw(AppleObjCRuntime::ObjCISA isa) {
+  Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS | LIBLLDB_LOG_TYPES));
+
+  ThreadSP thread_sp =
+  m_process->GetThreadList().GetExpressionExecutionThread();
+  if (!thread_sp)
+return llvm::make_error(
+"could not get the current thread", llvm::inconvertibleErrorCode());
+
+  ExecutionContext exe_ctx;
+  thread_sp->CalculateExecutionContext(exe_ctx);
+
+  TypeSystemClang *ast = TypeSystemClang::GetScratch(m_process->GetTarget());
+  if (!ast)
+return llvm::make_error(
+"could not get clang type system", llvm::inconvertibleErrorCode());
+
+  CompilerType clang_void_pointer_type =
+  ast->GetBasicType(eBasicTypeVoid).GetPointerType();
+  CompilerType clang_char_pointer_type =
+  ast->GetBasicType(eBasicTypeChar).GetPointerType();
+
+  FunctionCaller *debug_class_get_name_raw_function = nullptr;
+  Status error;
+  ValueList arguments;
+  DiagnosticManager diagnostics;
+
+  if (!m_debug_class_get_name_raw) {
+m_debug_class_get_name_raw.reset(
+GetTargetRef().GetUtilityFunctionForLanguage(
+g_debug_class_get_name_raw_body, eLanguageTypeObjC,
+g_debug_class_get_name_raw_name, error));
+
+if (error.Fail())
+  return error.ToError();
+
+if (!m_debug_class_get_name_raw->Install(diagnostics, exe_ctx)) {
+  if (log)
+diagnostics.Dump(log);
+  m_debug_class_get_name_raw.reset();
+  return llvm::make_error(
+  "could not install utility function", llvm::inconvertibleErrorCode());
+}
+
+Value value;
+value.SetValueType(Value::eValueTypeScalar);
+value.SetCompilerType(clang_void_pointer_type);
+arguments.PushValue(value);
+
+debug_class_get_name_raw_fun

[Lldb-commits] [lldb] a00acba - [lldb] Fix missing initialization in UtilityFunction ctor (NFC)

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-10-22T21:12:27-07:00
New Revision: a00acbab45b0c407da05bf5c8152018e1857a1f0

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

LOG: [lldb] Fix missing initialization in UtilityFunction ctor (NFC)

The UtilityFunction ctor was dropping the text argument. Probably for
that reason ClangUtilityFunction was setting the parent's member
directly instead of deferring to the parent ctor. Also change the
signatures to take strings which are std::moved in place.

Added: 


Modified: 
lldb/include/lldb/Expression/UtilityFunction.h
lldb/source/Expression/UtilityFunction.cpp
lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.h

Removed: 




diff  --git a/lldb/include/lldb/Expression/UtilityFunction.h 
b/lldb/include/lldb/Expression/UtilityFunction.h
index 5ebbc0ede1e3..99fb32153aa2 100644
--- a/lldb/include/lldb/Expression/UtilityFunction.h
+++ b/lldb/include/lldb/Expression/UtilityFunction.h
@@ -42,8 +42,8 @@ class UtilityFunction : public Expression {
   ///
   /// \param[in] name
   /// The name of the function, as used in the text.
-  UtilityFunction(ExecutionContextScope &exe_scope, const char *text,
-  const char *name);
+  UtilityFunction(ExecutionContextScope &exe_scope, std::string text,
+  std::string name);
 
   ~UtilityFunction() override;
 
@@ -110,9 +110,10 @@ class UtilityFunction : public Expression {
 protected:
   std::shared_ptr m_execution_unit_sp;
   lldb::ModuleWP m_jit_module_wp;
-  std::string m_function_text; ///< The text of the function.  Must be a
-   ///well-formed translation unit.
-  std::string m_function_name; ///< The name of the function.
+  /// The text of the function.  Must be a well-formed translation unit.
+  std::string m_function_text;
+  /// The name of the function.
+  std::string m_function_name;
   std::unique_ptr m_caller_up;
 };
 

diff  --git a/lldb/source/Expression/UtilityFunction.cpp 
b/lldb/source/Expression/UtilityFunction.cpp
index 3de2ee2acbbf..128db0ccbc3e 100644
--- a/lldb/source/Expression/UtilityFunction.cpp
+++ b/lldb/source/Expression/UtilityFunction.cpp
@@ -41,9 +41,9 @@ char UtilityFunction::ID;
 /// \param[in] name
 /// The name of the function, as used in the text.
 UtilityFunction::UtilityFunction(ExecutionContextScope &exe_scope,
- const char *text, const char *name)
+ std::string text, std::string name)
 : Expression(exe_scope), m_execution_unit_sp(), m_jit_module_wp(),
-  m_function_text(), m_function_name(name) {}
+  m_function_text(std::move(text)), m_function_name(std::move(name)) {}
 
 UtilityFunction::~UtilityFunction() {
   lldb::ProcessSP process_sp(m_jit_process_wp.lock());

diff  --git 
a/lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp 
b/lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
index 25ec982220a0..5fdb7b4f4d9c 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
@@ -42,11 +42,9 @@ char ClangUtilityFunction::ID;
 /// \param[in] name
 /// The name of the function, as used in the text.
 ClangUtilityFunction::ClangUtilityFunction(ExecutionContextScope &exe_scope,
-   const char *text, const char *name)
-: UtilityFunction(exe_scope, text, name) {
+   std::string text, std::string name)
+: UtilityFunction(exe_scope, std::move(text), std::move(name)) {
   m_function_text.assign(ClangExpressionSourceCode::g_expression_prefix);
-  if (text && text[0])
-m_function_text.append(text);
 }
 
 ClangUtilityFunction::~ClangUtilityFunction() {}

diff  --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.h 
b/lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.h
index 1f2dd5fdbecc..7914e1406cd0 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.h
@@ -41,6 +41,34 @@ class ClangUtilityFunction : public UtilityFunction {
   }
   static bool classof(const Expression *obj) { return obj->isA(&ID); }
 
+  /// Constructor
+  ///
+  /// \param[in] text
+  /// The text of the function.  Must be a full translation unit.
+  ///
+  /// \param[in] name
+  /// The name of the function, as used in the text.
+  ClangUtilityFunction(ExecutionContextScope &exe_scope, std::string text,
+   std::string name);
+
+  ~ClangUtilityFunction() override;
+
+  ExpressionTypeSystemHelper *GetTypeSy

[Lldb-commits] [lldb] 3590a83 - [lldb] Fix bug instroduced by a00acbab45b0

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-10-22T22:17:11-07:00
New Revision: 3590a8319a5fb491cba2349509910c2479f49a00

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

LOG: [lldb] Fix bug instroduced by a00acbab45b0

g_expression_prefix, as the name implies, must be perfixed, not
suffixed.

Added: 


Modified: 
lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp

Removed: 




diff  --git 
a/lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp 
b/lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
index 5fdb7b4f4d9c..9788a4e1c183 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
@@ -43,9 +43,10 @@ char ClangUtilityFunction::ID;
 /// The name of the function, as used in the text.
 ClangUtilityFunction::ClangUtilityFunction(ExecutionContextScope &exe_scope,
std::string text, std::string name)
-: UtilityFunction(exe_scope, std::move(text), std::move(name)) {
-  m_function_text.assign(ClangExpressionSourceCode::g_expression_prefix);
-}
+: UtilityFunction(
+  exe_scope,
+  std::string(ClangExpressionSourceCode::g_expression_prefix) + text,
+  std::move(name)) {}
 
 ClangUtilityFunction::~ClangUtilityFunction() {}
 



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

[Lldb-commits] [PATCH] D90011: [lldb] Redesign Target::GetUtilityFunctionForLanguage API

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: jingham, labath, LLDB.
Herald added a subscriber: emaste.
JDevlieghere requested review of this revision.

This patch redesigns the `Target::GetUtilityFunctionForLanguage` API:

- Use a `unique_ptr` instead of a raw pointer for the return type.
- Wrap the result in an `llvm::Expected` instead of using a `Status` object as 
an I/O parameter.
- Combine the action of "getting" and "installing" the UtilityFunction as they 
always get called together.
- Pass `std::string`s instead of `const char*` and `std::move` them where 
appropriate.

There's more room for improvement but I think this tackles the most prevalent 
issues with the current API.


https://reviews.llvm.org/D90011

Files:
  lldb/include/lldb/Symbol/TypeSystem.h
  lldb/include/lldb/Target/Target.h
  lldb/source/Plugins/ExpressionParser/Clang/IRDynamicChecks.cpp
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV1.cpp
  lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV1.h
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
  lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp
  lldb/source/Plugins/LanguageRuntime/ObjC/ObjCLanguageRuntime.h
  lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
  lldb/source/Plugins/SystemRuntime/MacOSX/AppleGetItemInfoHandler.cpp
  lldb/source/Plugins/SystemRuntime/MacOSX/AppleGetPendingItemsHandler.cpp
  lldb/source/Plugins/SystemRuntime/MacOSX/AppleGetQueuesHandler.cpp
  lldb/source/Plugins/SystemRuntime/MacOSX/AppleGetThreadItemInfoHandler.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
  lldb/source/Target/Target.cpp

Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -27,9 +27,11 @@
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Core/StructuredDataImpl.h"
 #include "lldb/Core/ValueObject.h"
+#include "lldb/Expression/DiagnosticManager.h"
 #include "lldb/Expression/ExpressionVariable.h"
 #include "lldb/Expression/REPL.h"
 #include "lldb/Expression/UserExpression.h"
+#include "lldb/Expression/UtilityFunction.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/PosixApi.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
@@ -2242,25 +2244,27 @@
   return persistent_fn;
 }
 
-UtilityFunction *
-Target::GetUtilityFunctionForLanguage(const char *text,
-  lldb::LanguageType language,
-  const char *name, Status &error) {
+llvm::Expected>
+Target::CreateUtilityFunction(std::string expression, std::string name,
+  lldb::LanguageType language,
+  ExecutionContext &exe_ctx) {
   auto type_system_or_err = GetScratchTypeSystemForLanguage(language);
+  if (!type_system_or_err)
+return type_system_or_err.takeError();
 
-  if (auto err = type_system_or_err.takeError()) {
-error.SetErrorStringWithFormat(
-"Could not find type system for language %s: %s",
-Language::GetNameForLanguageType(language),
-llvm::toString(std::move(err)).c_str());
-return nullptr;
-  }
-
-  auto *utility_fn = type_system_or_err->GetUtilityFunction(text, name);
+  std::unique_ptr utility_fn =
+  type_system_or_err->GetUtilityFunction(std::move(expression),
+ std::move(name));
   if (!utility_fn)
-error.SetErrorStringWithFormat(
-"Could not create an expression for language %s",
-Language::GetNameForLanguageType(language));
+return llvm::make_error(
+llvm::StringRef("Could not create an expression for language") +
+Language::GetNameForLanguageType(language),
+llvm::inconvertibleErrorCode());
+
+  DiagnosticManager diagnostics;
+  if (!utility_fn->Install(diagnostics, exe_ctx))
+return llvm::make_error(diagnostics.GetString(),
+   llvm::inconvertibleErrorCode());
 
   return utility_fn;
 }
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -1134,8 +1134,8 @@
 const ValueList &arg_value_list,
 const char *name) override;
 
-  UtilityFunction *GetUtilityFunction(const char *text,
-  const char *name) override;
+  std::unique_ptr
+  GetUtilityFunction(std::string text, std::string name) override;
 
   PersistentExpressionState *GetPersistentExpressionState() override;
 private:
Index: ll