[Lldb-commits] [PATCH] D110172: [lldb/win] Default to native PDB reader when building with LLVM_ENABLE_DIA_SDK=NO

[Adrian McCarthy via Phabricator via lldb-commits]

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

LGTM


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

https://reviews.llvm.org/D110172

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

[Lldb-commits] [PATCH] D109834: [lldb/win] Improve check-lldb-shell with LLVM_ENABLE_DIA_SDK=NO

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

I'm trying to understand why I've never seen the problem that this patch is 
trying to address.

I wasn't aware of `LLVM_ENABLE_DIA_SDK` so I've never turned it off.  But I've 
run `check lldb` exclusively with `LLDB_USE_NATIVE_PDB_READER=TRUE` for a long 
time, so I'm surprised to hear that some tests somehow still depend on DIA.  Is 
`check-lldb-shell` not a subset of `check-lldb`?

How exactly would I reproduce the problem here?

I don't find much meaning in the term `host-breakpoints`, but maybe that's 
because I don't understand the bug.


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

https://reviews.llvm.org/D109834

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

[Lldb-commits] [PATCH] D109832: [lldb/win] Fix TestIRMemoryMapWindows.test when running tests in git bash

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109832

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

[Lldb-commits] [PATCH] D104954: Implement AddSymbols method for SymbolFileNativePDB

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth created this revision.
amccarth added a reviewer: rnk.
amccarth requested review of this revision.

In theory, this adds symbols from the module's PDB file to the symtab.  This 
implementation was modeled after the one in SymbolFilePDB, so it relies on the 
same IPDBSession abstraction as the DIA-based PDB reader. 
 Unfortunately, the native reader implementation of that interface was not 
fully implemented, so the attempt to find the public symbols comes up empty.  
The result is that this patch has no functional change (and thus there are no 
meaningful tests to write).

  

I am currently investigating whether to implement the abstraction or to use the 
lower level native PDB reader interface directly in SymbolFileNativePDB.


https://reviews.llvm.org/D104954

Files:
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp


Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -48,6 +48,8 @@
 #include "llvm/DebugInfo/PDB/Native/SymbolStream.h"
 #include "llvm/DebugInfo/PDB/Native/TpiStream.h"
 #include "llvm/DebugInfo/PDB/PDB.h"
+#include "llvm/DebugInfo/PDB/PDBSymbolPublicSymbol.h"
+#include "llvm/DebugInfo/PDB/PDBSymbolExe.h"
 #include "llvm/DebugInfo/PDB/PDBTypes.h"
 #include "llvm/Demangle/MicrosoftDemangle.h"
 #include "llvm/Object/COFF.h"
@@ -307,8 +309,8 @@
   auto ts_or_err = m_objfile_sp->GetModule()->GetTypeSystemForLanguage(
   lldb::eLanguageTypeC_plus_plus);
   if (auto err = ts_or_err.takeError()) {
-LLDB_LOG_ERROR(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_SYMBOLS),
-   std::move(err), "Failed to initialize");
+Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_SYMBOLS));
+LLDB_LOG_ERROR(log, std::move(err), "Failed to initialize");
   } else {
 ts_or_err->SetSymbolFile(this);
 auto *clang = llvm::cast_or_null(_or_err.get());
@@ -905,7 +907,63 @@
   return TranslateLanguage(item->m_compile_opts->getLanguage());
 }
 
-void SymbolFileNativePDB::AddSymbols(Symtab ) { return; }
+void SymbolFileNativePDB::AddSymbols(Symtab ) {
+  // Currently, we can handle only one symbol for any given address, so we'll
+  // use a `std::set` to keep track of known addresses.
+  std::set sym_addresses;
+  for (size_t i = 0; i < symtab.GetNumSymbols(); i++) {
+sym_addresses.insert(symtab.SymbolAtIndex(i)->GetFileAddress());
+  }
+
+  std::unique_ptr session_up;
+  auto error = llvm::pdb::loadDataForPDB(PDB_ReaderType::Native,
+  GetPDBFile().getFilePath(), session_up);
+  if (error) return;
+  NativeSession *native_session = static_cast(session_up.get());
+  auto global_scope_up = native_session->getGlobalScope();
+
+  auto results = global_scope_up->findAllChildren();
+  if (!results)
+return;
+
+  auto section_list = m_objfile_sp->GetSectionList();
+  if (!section_list)
+return;
+
+  while (auto pub_symbol = results->getNext()) {
+auto section_id = pub_symbol->getAddressSection();
+
+auto section = section_list->FindSectionByID(section_id);
+if (!section)
+  continue;
+
+auto offset = pub_symbol->getAddressOffset();
+
+auto file_addr = section->GetFileAddress() + offset;
+if (sym_addresses.find(file_addr) != sym_addresses.end())
+  continue;
+sym_addresses.insert(file_addr);
+
+auto size = pub_symbol->getLength();
+symtab.AddSymbol(
+Symbol(pub_symbol->getSymIndexId(),
+   pub_symbol->getName().c_str(),
+   pub_symbol->isCode() ? eSymbolTypeCode : eSymbolTypeData,
+   /* external */ true,
+   /* is_debug */ false,
+   /* is_trampoline */ false,
+   /* is_artificial */ false,
+   section,
+   offset,
+   size,
+   /* size_is_valid */ size != 0,
+   /* contains_linker_annotations */ false,
+   /* flags */ 0));
+  }
+
+  symtab.CalculateSymbolSizes();
+  symtab.Finalize();
+}
 
 size_t SymbolFileNativePDB::ParseFunctions(CompileUnit _unit) {
   std::lock_guard guard(GetModuleMutex());


Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -48,6 +48,8 @@
 #include "llvm/DebugInfo/PDB/Native/SymbolStream.h"
 #include "llvm/DebugInfo/PDB/Native/TpiStream.h"
 #include "llvm/DebugInfo/PDB/PDB.h"
+#include "llvm/DebugInfo/PDB/PDBSymbolPublicSymbol.h"
+#include "llvm/DebugInfo/PDB/PDBSymbolExe.h"
 #include "llvm/DebugInfo/PDB/PDBTypes.h"
 #include "llvm/Demangle/MicrosoftDemangle.h"
 #include "llvm/Object/COFF.h"
@@ -307,8 +309,8 @@
   auto ts_or_err = 

[Lldb-commits] [PATCH] D102208: Remove Windows editline from LLDB

[Adrian McCarthy via Phabricator via lldb-commits]

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

LGTM.  Dead code should go.  If somebody wants to own a Windows port of 
EditLine, they can re-instate it and put some tests on it.

Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102208

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

[Lldb-commits] [PATCH] D102208: Remove Windows editline from LLDB

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a subscriber: zturner.
amccarth added a comment.

It looks like this EditLine port was added before I joined this project.  
@zturner may have worked on it, but I don't know/remember.

If it's actually unused, I have no objection to removing it.  I harbor some 
hope that Windows's steadily improving unix-like terminal implementation (which 
console programs can enable) will eventually make a Windows-port of EditLine 
trivial.

I concur that the MSVC version tweak should be a separate change.  If you 
remove that from this patch, I'll LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102208

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

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

I like the composability enabled by making `OF_Text` and `OF_CRLF` independent. 
 I wonder, though, if there's a chokepoint where we could/should assert if the 
caller uses `OF_CRLF` without `OF_Text`, which would be suspicious.

I'm not concerned by the number of files touched, since it's mostly a 
mechanical refactor.  If anyone's worried that the mechanical changes obscure 
the behavior change, this _could_ be broken into an NFC refactor patch for the 
renaming followed by a patch that implements the behavioral distinction.  But 
I'm not going to insist on that.




Comment at: llvm/include/llvm/Support/FileSystem.h:746
   /// The file should be opened in text mode on platforms that make this
   /// distinction.
   OF_Text = 1,

Don't be shy to give examples in the comments, as they can illuminate the 
motivation for the design.

```
... on platforms, like SystemZ, that distinguish between text and binary files.
```



Comment at: llvm/include/llvm/Support/FileSystem.h:757
+  /// OF_TextWithCRLF = OF_Text | OF_CRLF
+  OF_TextWithCRLF = 3,
+

Nit:  If it were me, I'd put the `OF_Text | OF_CRLF` directly in the code (in 
place of the `3`) instead of in the comment.



Comment at: llvm/lib/Support/Windows/Path.inc:1086
 
-  if (Flags & OF_Text)
+  if (Flags & OF_CRLF)
 CrtOpenFlags |= _O_TEXT;

I assume it would be weird to set `OF_CRLF` without also setting `OF_Text`, so 
this change seems right for correctness.  But maybe this Windows-only code 
would better express the intent if it were written:

```
if (Flags & (OF_CRLF | OF_Text))
```

Another idea would be to assert if `OF_CRLF` is set but `OF_Text` is not.

Or maybe it's fine as it is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426

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

[Lldb-commits] [PATCH] D98529: [lldb] Strip pointer authentication codes from aarch64 pc.

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

Minidumps should have the registers in the processor context.  It seems LLDB 
knows about TCR_ELn for n > 0.  Maybe TCR_EL0 is special?

If it's not available in the minidump, we'll need a plan for how to deal with 
these regardless of when Jason's implementation lands.

Before reading Jason's response, I was independently wondering whether it makes 
sense to temporarily introduce a variable to let the user set the mask, just 
until the workaround is replaced with final code.  Given that there's 
precedent, I would support that.  (I've not implemented one of these LLDB 
settings before, but I imagine it's pretty straightforward.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98529

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

[Lldb-commits] [PATCH] D98529: [lldb] Strip pointer authentication codes from aarch64 pc.

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

This workaround seems consistent with other overrides of FixCodeAddress, my 
only concern is about the assumption of the number of bits that need to be 
preserved/stripped.

I hope @jasonmolenda gets a chance to chime in.




Comment at: lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.h:68
+// should be removed once full PAC support is added.
+return pc & 0x000F;
+  }

Is that mask correct?  If I'm counting correctly, it looks like it's preserving 
the lower 36 bits.  I thought most 64-bit CPUs are configured to handle up to 
40 or 48 bits of virtual address space.

I assume that future PAC support will check the CPU configuration, but is 36 
bits the right assumption for this workaround?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98529

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

[Lldb-commits] [PATCH] D96840: [LLDB] [docs] Update the list of supported architectures on Windows

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

I'm in no position to evaluate the functionality of lldb on Windows.  I've 
spent the last few months trying to understand why 900+ tests spontaneously 
started failing on Windows.


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

https://reviews.llvm.org/D96840

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

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

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

This looks pretty good, both the patch and Pavel's insights.  I don't see much 
to comment on that Pavel didn't already catch.




Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:168
+
+  ArchSpec  = module_spec.GetArchitecture();
+  switch (dbi_stream->getMachineType()) {

For me, the name `spec` is confusing, because this code is mostly dealing with 
ModuleSpecs but `spec` is a reference to the ArchSpec of the module_spec.  
Perhaps `module_arch` would make this clearer.  Then the code below would 
follow a pattern like:

module_arch.SetTriple("blah");
specs.Append(module_spec);



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89812

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

[Lldb-commits] [PATCH] D89256: [LLDB] Fix 37 tests on Windows

[Adrian McCarthy via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf21fcccef719: [LLDB] Fix 37 tests on Windows (authored by 
amccarth).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89256

Files:
  lldb/include/lldb/Host/Config.h.cmake


Index: lldb/include/lldb/Host/Config.h.cmake
===
--- lldb/include/lldb/Host/Config.h.cmake
+++ lldb/include/lldb/Host/Config.h.cmake
@@ -52,7 +52,7 @@
 
 #cmakedefine01 LLDB_EMBED_PYTHON_HOME
 
-#cmakedefine LLDB_PYTHON_HOME "${LLDB_PYTHON_HOME}"
+#cmakedefine LLDB_PYTHON_HOME R"(${LLDB_PYTHON_HOME})"
 
 #define LLDB_LIBDIR_SUFFIX "${LLVM_LIBDIR_SUFFIX}"
 


Index: lldb/include/lldb/Host/Config.h.cmake
===
--- lldb/include/lldb/Host/Config.h.cmake
+++ lldb/include/lldb/Host/Config.h.cmake
@@ -52,7 +52,7 @@
 
 #cmakedefine01 LLDB_EMBED_PYTHON_HOME
 
-#cmakedefine LLDB_PYTHON_HOME "${LLDB_PYTHON_HOME}"
+#cmakedefine LLDB_PYTHON_HOME R"(${LLDB_PYTHON_HOME})"
 
 #define LLDB_LIBDIR_SUFFIX "${LLVM_LIBDIR_SUFFIX}"
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D89256: [LLDB] Fix 37 tests on Windows

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth created this revision.
amccarth added a reviewer: JDevlieghere.
Herald added a subscriber: mgorny.
amccarth requested review of this revision.

A Windows-style LLDB_PYTHON_HOME path in a Cmake template didn't have the 
backslashes escaped, which led to a garbled paths derived from it.  Fixed by 
expanding the environment variable as a raw string literal.


https://reviews.llvm.org/D89256

Files:
  lldb/include/lldb/Host/Config.h.cmake


Index: lldb/include/lldb/Host/Config.h.cmake
===
--- lldb/include/lldb/Host/Config.h.cmake
+++ lldb/include/lldb/Host/Config.h.cmake
@@ -52,7 +52,7 @@
 
 #cmakedefine01 LLDB_EMBED_PYTHON_HOME
 
-#cmakedefine LLDB_PYTHON_HOME "${LLDB_PYTHON_HOME}"
+#cmakedefine LLDB_PYTHON_HOME R"(${LLDB_PYTHON_HOME})"
 
 #define LLDB_LIBDIR_SUFFIX "${LLVM_LIBDIR_SUFFIX}"
 


Index: lldb/include/lldb/Host/Config.h.cmake
===
--- lldb/include/lldb/Host/Config.h.cmake
+++ lldb/include/lldb/Host/Config.h.cmake
@@ -52,7 +52,7 @@
 
 #cmakedefine01 LLDB_EMBED_PYTHON_HOME
 
-#cmakedefine LLDB_PYTHON_HOME "${LLDB_PYTHON_HOME}"
+#cmakedefine LLDB_PYTHON_HOME R"(${LLDB_PYTHON_HOME})"
 
 #define LLDB_LIBDIR_SUFFIX "${LLVM_LIBDIR_SUFFIX}"
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D88967: [lldb] Add a cmake warning about the python/swig incompatibility

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

In D88967#2317545 , @labath wrote:

> In D88967#2317522 , @amccarth wrote:
>
>> If I recall correctly, the non-debug builds still had the problem, they just 
>> didn't have the assertion that made it obvious.
>
> Is that problem only theoretical (like, "you shouldn't be doing that") or 
> does it have some practical consequences (crashes, incorrect operation, etc.)?

My memory isn't that detailed.  I think it was an argument being passed in as a 
single value but being referenced as though it was an array or list.  Or vice 
versa.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88967

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

[Lldb-commits] [PATCH] D88975: [LLDB] On Windows, fix tests

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

It's interesting that I haven't encountered some of these errors.  There are 
five _other_ lldb tests that do fail for me.  I have a fix in the works for 
some of those.

I agree with @labath that including error message patterns in various languages 
isn't scalable. Since dotest,py is starting up the processes under test, 
perhaps there's a way to force it to use a particular locale.  Besides the 
language of system error messages, locales can change the format of numbers, 
dates, etc.

Unfortunately, I don't think it's as simple as an environment variable.  I 
expect this is driven by the user's (or system default) locales settings as 
tracked by Windows.  That's distinct from the C runtime concept of locales.

Maybe dotest.py could change its own Windows locale setting and the processes 
it spawns would inherit that.  I don't know if that would work, but I don't see 
a good alternative.




Comment at: lldb/unittests/Utility/StatusTest.cpp:80
+  if (wcscmp(L"en-US", name) != 0)
+return;
+

Rather than an early return, perhaps the code should still be exercised, but 
the language-specific EXPECTs could be skipped.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88975

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

[Lldb-commits] [PATCH] D88906: [lldb/docs] Clarify python/swig version incompatibility

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth accepted this revision.
amccarth added a comment.

Yes, I also (independently) discovered this problem.  I probably have notes 
somewhere with sources for details about the incompatibility.  I believe I also 
brought it up on the lldb-dev list.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88906

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

[Lldb-commits] [PATCH] D85993: [lldb] Set the access property on member function decls

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

In D85993#2220724 , @labath wrote:

> Dwarf parser uses `TypeSystemClang::AddMethodToCXXRecordType` instead of this 
> function to create methods (which is why there are no assertions like this 
> when using dwarf). Maybe it would be better to change the pdb parser to use 
> that function instead (as it allows the user to specify not only 
> accessibility, but also other potentially useful properties of the created 
> method -- static, virtual, etc.).

The NativePDB AST parser also uses `AddMethodToCXXRecordType`.  It's in 
udtcompleter.cpp.

But the bug is triggered while the plugin is trying to synthesize the function 
declaration during a `ResolveSymbolContext` call.  It looks like the DWARF 
implementation of `ResolveSymbolContext` relies on its AST parser, but the 
NativePDB one does not.  I'm trying to figure out how to do that.

Also note that `AddMethodToCXXRecordType` just sets whatever access type it's 
asked to set.  Before calling `AddMethodToCXXRecordType`, the DWARF parser has 
this gem:

  // Neither GCC 4.2 nor clang++ currently set a valid
  // accessibility in the DWARF for C++ methods...
  // Default to public for now...
  if (attrs.accessibility == eAccessNone)
attrs.accessibility = eAccessPublic;

That bit of logic is what prevents the assertion from failing for DWARF.  I'll 
can make the NativePDB AST parser do the same thing.

But that won't fix the bug, since the NativePDB AST parser isn't involved (at 
least, my breakpoints in the parser never hit during the course of the test).  
In the mean time, I'll try to figure out how to get the NativePDB plugin's 
`ResolveSymbolContext` implementation to use `AddMethodToCXXRecord`.

> And this function could assert that it is _only_ used for creating free 
> functions ?

Sure.  Sounds like a good idea.

I'll also assume that, since you don't want my access-fixup in 
`TypeSystemClang::CreateFunctionDeclaration`, then you also would want me to 
remove the already-existing such fixup from 
`TypeSystemClang::CreateFunctionTemplateDecl`.  Does that make sense?


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

https://reviews.llvm.org/D85993

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

[Lldb-commits] [PATCH] D85993: [lldb] Set the access property on member function decls

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth created this revision.
amccarth added reviewers: clayborg, aganea.
amccarth requested review of this revision.

This fixes two failures in the PDB tests.  LLVM has a "sanity" check on 
function decls.  One of the requirements of member functions is that they have 
the access property (public, protected, private) set if the function is a 
member function.  The check is an assert, so you'll see the failure only if 
you're running with assertions enabled.

This sets the access property to public to match how the existing code handles 
member function templates.


https://reviews.llvm.org/D85993

Files:
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp


Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -2017,6 +2017,12 @@
   func_decl->setHasWrittenPrototype(hasWrittenPrototype);
   func_decl->setConstexprKind(isConstexprSpecified ? CSK_constexpr
: CSK_unspecified);
+  // Functions inside a record need to have an access specifier.  It doesn't
+  // matter what access specifier we give the function as LLDB allows
+  // accessing everything inside a record.
+  if (decl_ctx->isRecord())
+func_decl->setAccess(clang::AccessSpecifier::AS_public);
+
   SetOwningModule(func_decl, owning_module);
   if (func_decl)
 decl_ctx->addDecl(func_decl);


Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -2017,6 +2017,12 @@
   func_decl->setHasWrittenPrototype(hasWrittenPrototype);
   func_decl->setConstexprKind(isConstexprSpecified ? CSK_constexpr
: CSK_unspecified);
+  // Functions inside a record need to have an access specifier.  It doesn't
+  // matter what access specifier we give the function as LLDB allows
+  // accessing everything inside a record.
+  if (decl_ctx->isRecord())
+func_decl->setAccess(clang::AccessSpecifier::AS_public);
+
   SetOwningModule(func_decl, owning_module);
   if (func_decl)
 decl_ctx->addDecl(func_decl);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D84815: [LLDB] Improve PDB discovery

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth closed this revision.
amccarth added a comment.

Landed on Tuesday.  I must have messed up the `Differential Revision:` line.


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

https://reviews.llvm.org/D84815

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

[Lldb-commits] [PATCH] D84815: [LLDB] Improve PDB discovery

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth updated this revision to Diff 284025.
amccarth added a comment.

Added test to check locating the PDB either in the original build directory or 
adjacent to the executable.

I tried but failed to make a negative test.  LLDB sends the errors message to 
stderr when the `target modules dump symfile` command fails.  I tried using 
redirects to combine stdout and stderr, but lit's built-in shell told me that 
was unsupported.  I couldn't find a good way to arrange FileCheck to confirm 
the absence case.


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

https://reviews.llvm.org/D84815

Files:
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/test/Shell/SymbolFile/NativePDB/Inputs/locate-pdb.lldbinit
  lldb/test/Shell/SymbolFile/NativePDB/locate-pdb.cpp


Index: lldb/test/Shell/SymbolFile/NativePDB/locate-pdb.cpp
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/NativePDB/locate-pdb.cpp
@@ -0,0 +1,34 @@
+// clang-format off
+// REQUIRES: lld, x86
+
+// Test that lldb can find the PDB file that corresponds to the executable.  
The linker
+// writes a path to the PDB in the executable.  If the PDB is not there, lldb 
should
+// check the directory that contains the executable.  We'll generate the PDB 
file in
+// a subdirectory and then move it into the directory with the executable.  
That will
+// ensure the PDB path stored in the executable is wrong.
+
+// Build an EXE and PDB in different directories
+// RUN: mkdir -p %t/executable
+// RUN: rm -f %t/executable/foo.exe %t/executable/bar.pdb
+// RUN: mkdir -p %t/symbols
+// RUN: rm -f %t/symbols/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/symbols/bar.pdb
+
+// Find the PDB in its build location
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t/executable/foo.exe -s \
+// RUN: %p/Inputs/locate-pdb.lldbinit | FileCheck %s
+
+// Also find the PDB when it's adjacent to the executable
+// RUN: mv -f %t/symbols/bar.pdb %t/executable/bar.pdb
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t/executable/foo.exe -s \
+// RUN: %p/Inputs/locate-pdb.lldbinit | FileCheck %s
+
+int main(int argc, char** argv) {
+  return 0;
+}
+
+// CHECK: (lldb) target modules dump symfile
+// CHECK: Dumping debug symbols for 1 modules.
+// CHECK: SymbolFile pdb
Index: lldb/test/Shell/SymbolFile/NativePDB/Inputs/locate-pdb.lldbinit
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/NativePDB/Inputs/locate-pdb.lldbinit
@@ -0,0 +1,2 @@
+target modules dump symfile
+quit
Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -134,8 +134,16 @@
 return nullptr;
   }
 
-  // if the file doesn't exist, is not a pdb, or doesn't have a matching guid,
-  // fail.
+  // If the file doesn't exist, perhaps the path specified at build time
+  // doesn't match the PDB's current location, so check the location of the
+  // executable.
+  if (!FileSystem::Instance().Exists(pdb_file)) {
+const auto exe_dir = FileSpec(exe_path).CopyByRemovingLastPathComponent();
+const auto pdb_name = FileSpec(pdb_file).GetFilename().GetCString();
+pdb_file = exe_dir.CopyByAppendingPathComponent(pdb_name).GetCString();
+  }
+
+  // If the file is not a PDB or if it doesn't have a matching GUID, fail.
   llvm::file_magic magic;
   auto ec = llvm::identify_magic(pdb_file, magic);
   if (ec || magic != llvm::file_magic::pdb)


Index: lldb/test/Shell/SymbolFile/NativePDB/locate-pdb.cpp
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/NativePDB/locate-pdb.cpp
@@ -0,0 +1,34 @@
+// clang-format off
+// REQUIRES: lld, x86
+
+// Test that lldb can find the PDB file that corresponds to the executable.  The linker
+// writes a path to the PDB in the executable.  If the PDB is not there, lldb should
+// check the directory that contains the executable.  We'll generate the PDB file in
+// a subdirectory and then move it into the directory with the executable.  That will
+// ensure the PDB path stored in the executable is wrong.
+
+// Build an EXE and PDB in different directories
+// RUN: mkdir -p %t/executable
+// RUN: rm -f %t/executable/foo.exe %t/executable/bar.pdb
+// RUN: mkdir -p %t/symbols
+// RUN: rm -f %t/symbols/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/symbols/bar.pdb
+
+// Find 

[Lldb-commits] [PATCH] D84815: [LLDB] Improve PDB discovery

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

Thanks.  Working on a test.

I found a description of how the Windows debuggers look for PDBs, and it's 
different than what I expected:

1. The directory that contains the binary
2. The build path embedded in the binary
3. (if enabled) The symbol server cache
4. (if enabled) That symbol server

LLDB currently has only step 2.  This patch adds step 1, but it adds it as the 
_second_ step.  And that's it.  I figured it might also look locally on the 
PATH or some "well-known" locations, but nope.

(It's buried in a paragraph in this long article by John Robbins:  
https://www.wintellect.com/pdb-files-what-every-developer-must-know/)


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

https://reviews.llvm.org/D84815

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

[Lldb-commits] [PATCH] D84815: [LLDB] Improve PDB discovery

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth updated this revision to Diff 281445.
amccarth added a comment.

Fixed typos in comments.


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

https://reviews.llvm.org/D84815

Files:
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp


Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -134,8 +134,16 @@
 return nullptr;
   }
 
-  // if the file doesn't exist, is not a pdb, or doesn't have a matching guid,
-  // fail.
+  // If the file doesn't exist, perhaps the path specified at build time
+  // doesn't match the PDB's current location, so check the location of the
+  // executable.
+  if (!FileSystem::Instance().Exists(pdb_file)) {
+const auto exe_dir = FileSpec(exe_path).CopyByRemovingLastPathComponent();
+const auto pdb_name = FileSpec(pdb_file).GetFilename().GetCString();
+pdb_file = exe_dir.CopyByAppendingPathComponent(pdb_name).GetCString();
+  }
+
+  // If the file is not a PDB or if it doesn't have a matching GUID, fail.
   llvm::file_magic magic;
   auto ec = llvm::identify_magic(pdb_file, magic);
   if (ec || magic != llvm::file_magic::pdb)


Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -134,8 +134,16 @@
 return nullptr;
   }
 
-  // if the file doesn't exist, is not a pdb, or doesn't have a matching guid,
-  // fail.
+  // If the file doesn't exist, perhaps the path specified at build time
+  // doesn't match the PDB's current location, so check the location of the
+  // executable.
+  if (!FileSystem::Instance().Exists(pdb_file)) {
+const auto exe_dir = FileSpec(exe_path).CopyByRemovingLastPathComponent();
+const auto pdb_name = FileSpec(pdb_file).GetFilename().GetCString();
+pdb_file = exe_dir.CopyByAppendingPathComponent(pdb_name).GetCString();
+  }
+
+  // If the file is not a PDB or if it doesn't have a matching GUID, fail.
   llvm::file_magic magic;
   auto ec = llvm::identify_magic(pdb_file, magic);
   if (ec || magic != llvm::file_magic::pdb)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D84815: [LLDB] Improve PDB discovery

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth created this revision.
amccarth added reviewers: labath, stella.stamenova.
amccarth requested review of this revision.

When loading a PE/COFF target, the associated PDB file often wasn't found.  The 
executable module contains a path for the associated PDB file, but people often 
debug from a different directory than the one their build system uses.  (This 
is especially common in post-mortem and cross platform debugging.)

Suppose the COFF executable being debugged is `~/proj/foo.exe`, but it was 
built elsewhere and refers to `D:\remote\build\env\foobar.pdb`, LLDB wouldn't 
find it.

With this change, if no file exists at the PDB path, LLDB will look in the 
executable directory for a PDB file that matches the name of the one it 
expected (e.g., `~/proj/foobar.pdb`).  If found, the PDB is subject to the same 
matching criteria (GUIDs and age) as would have been used had it been in the 
original location.

This same-directory-as-the-binary rule is commonly used by debuggers on Windows.


https://reviews.llvm.org/D84815

Files:
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp


Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -134,8 +134,16 @@
 return nullptr;
   }
 
-  // if the file doesn't exist, is not a pdb, or doesn't have a matching guid,
-  // fail.
+  // If the files doesn't exist, perhaps the path specified at build time
+  // doesn't match the PDB's current location, so check the location of the
+  // executable.
+  if (!FileSystem::Instance().Exists(pdb_file)) {
+const auto exe_dir = FileSpec(exe_path).CopyByRemovingLastPathComponent();
+const auto pdb_name = FileSpec(pdb_file).GetFilename().GetCString();
+pdb_file = exe_dir.CopyByAppendingPathComponent(pdb_name).GetCString();
+  }
+
+  // If the file is not a PDB of if it doesn't have a matching GUID, fail.
   llvm::file_magic magic;
   auto ec = llvm::identify_magic(pdb_file, magic);
   if (ec || magic != llvm::file_magic::pdb)


Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -134,8 +134,16 @@
 return nullptr;
   }
 
-  // if the file doesn't exist, is not a pdb, or doesn't have a matching guid,
-  // fail.
+  // If the files doesn't exist, perhaps the path specified at build time
+  // doesn't match the PDB's current location, so check the location of the
+  // executable.
+  if (!FileSystem::Instance().Exists(pdb_file)) {
+const auto exe_dir = FileSpec(exe_path).CopyByRemovingLastPathComponent();
+const auto pdb_name = FileSpec(pdb_file).GetFilename().GetCString();
+pdb_file = exe_dir.CopyByAppendingPathComponent(pdb_name).GetCString();
+  }
+
+  // If the file is not a PDB of if it doesn't have a matching GUID, fail.
   llvm::file_magic magic;
   auto ec = llvm::identify_magic(pdb_file, magic);
   if (ec || magic != llvm::file_magic::pdb)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D84070: [LLDB] [COFF] Fix handling of symbols with more than one aux symbol

[Adrian McCarthy via Phabricator via lldb-commits]

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

LGTM.  That was a nice catch on the other review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84070



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

[Lldb-commits] [PATCH] D83881: [lldb/COFF] Remove strtab zeroing hack

[Adrian McCarthy via Phabricator via lldb-commits]

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

Thanks @mstorsjo for clarifying for me.




Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:642
 DataExtractor symtab_data =
 ReadImageData(m_coff_header.symoff, symbol_data_size + 4);
 lldb::offset_t offset = symbol_data_size;

mstorsjo wrote:
> amccarth wrote:
> > The `+4` at the end of this expression is from the same patch.  I wonder if 
> > it was an attempt to make space for the four bytes of zeros at offset 0 
> > that you're eliminating?
> > 
> > I suggest removing the `+4` and trying the tests again unless it's obvious 
> > to you why it's still necessary.  The comment above seems like it might be 
> > trying to explain it, but that comment came later.
> No, the `+4` here was present before 0076e71592a6a (if viewing that commit, 
> view it with a bit more than the default git context size and you'll find "// 
> Include the 4 bytes string table size at the end of the symbols" existing 
> already before that.
> 
> The +4 here can't be eliminated; without it, the `const uint32_t strtab_size 
> = symtab_data.GetU32()` two lines below would be out of range. So this 
> first reads the symbol table and the 4 byte size of the string table, and if 
> the string table turns out to be nonzero in size, it also loads a separate 
> DataExtractor with the string table contents, with the two DataExtractors 
> overlapping for that size field. It doesn't have anything to do with 
> overwriting the start, just with the code layout in general.

Sorry, I mixed up `strtab_data` and `symtab_data` when comparing to the old 
patch, which is why I didn't see the comment where I expected it.

The old patch actually _removed_ a `+4` when computing the offset for 
`strtab_data`.  It seemed weird this patch didn't have to restore that in order 
to back out this part of the change.  But I think I understand it now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83881



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

[Lldb-commits] [PATCH] D83881: [lldb/COFF] Remove strtab zeroing hack

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

Yes, getting rid of this hack looks like a good idea.  If it was actually 
necessary, there should have been a test on it, and the comments should have 
been clearer.

See my inline comment, though.  It looks like this might back out only part of 
the change.




Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:642
 DataExtractor symtab_data =
 ReadImageData(m_coff_header.symoff, symbol_data_size + 4);
 lldb::offset_t offset = symbol_data_size;

The `+4` at the end of this expression is from the same patch.  I wonder if it 
was an attempt to make space for the four bytes of zeros at offset 0 that 
you're eliminating?

I suggest removing the `+4` and trying the tests again unless it's obvious to 
you why it's still necessary.  The comment above seems like it might be trying 
to explain it, but that comment came later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83881



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

[Lldb-commits] [PATCH] D81058: [lldb/Interpreter] Color "error:" and "warning:" in the CommandReturnObject output.

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

I'm just responding to the questions @labath raised without really looking at 
the details of the code.

I agree that there's nothing wrong with having the ability to output color 
codes to a file, but it's a surprising default and experience tells me it would 
break a lot of tests that are rightfully looking just at the content rather 
than the styling.  But having the ability to turn on color for file output 
would make it possible to also test styling, an ability we don't currently have.

Plumbing the option for color and other niceties down to a low level sounds 
like a great idea, as long as we get the defaults right.  For Windows (for now) 
and for output redirected to a file, the default should be off.  That said, I 
could see lowering of the color option being a separate effort outside the 
scope of this patch.  For one thing, we'd have to get buy-in from the more 
general LLVM folks.  And several of the llvm utilities currently use the 
`WithColor` approach, so there could be a lot of work updating those (or they 
would continue to use the old approach indefinitely).


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D81058



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

[Lldb-commits] [PATCH] D77662: [lldb/test] Make TestLoadUnload compatible with windows

[Adrian McCarthy via Phabricator via lldb-commits]

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

Well, it looks to go _me_.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77662



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

[Lldb-commits] [PATCH] D77662: [lldb/test] Make TestLoadUnload compatible with windows

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/make/Makefile.rules:477
 ifeq (1,$(USE_LIBDL))
-   ifneq ($(OS),NetBSD)
+   ifeq (,$(filter $(OS), NetBSD Windows_NT))
LDFLAGS += -ldl

I'm no expert in makefile syntax (especially not for Unix-y versions), but this 
looks weird, with the leading comma and the matching parenthesis for `$(filter` 
sitting near the end of the expression.  Is this right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77662



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

[Lldb-commits] [PATCH] D77662: [WIP][lldb/test] Make TestLoadUnload compatible with windows

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

I like where this is going.




Comment at: lldb/packages/Python/lldbsuite/test/make/dylib.h:50
+  FormatMessageA(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM,
+  NULL, err, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), (char *), 0, 
NULL);
+  return msg;

This leaks the buffer allocated for the message.  I guess we don't expect this 
to happen often, so maybe that's not a big deal, and it's a terrible API to 
have to deal with.



Comment at: lldb/test/API/functionalities/load_unload/TestLoadUnload.py:209
+env_cmd_string = "settings set target.env-vars " + self.dylibPath + 
"=" + self.getBuildArtifact(".")
+self.runCmd(env_cmd_string)
+

The env_cmd_string is going to eliminate ALL of the environment variables for 
the target except the one(s) that it explicitly sets.  Is that what you 
intended?

The lldb help says:
> Warning:  The 'set' command re-sets the entire array or dictionary.  If you 
> just want to add, remove or update individual values (or add something to the 
> end), use one of the other settings sub-commands: append, replace, 
> insert-before or insert-after.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77662



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

[Lldb-commits] [PATCH] D76835: [lldb] Fix TestSettings.test_pass_host_env_vars on windows

[Adrian McCarthy via Phabricator via lldb-commits]

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

The code is correct, so I'm approving, but I suggest a couple fixes to the 
comments before committing..




Comment at: lldb/source/Host/windows/ProcessLauncherWindows.cpp:27
+  // Environment buffer is a list of null-terminated list of strings. It ends
+  // with a double null.
   for (const auto  : env) {

You have an extra "list of" between null-terminated and "strings".  And I think 
we should point out that it's UTF-16 encoded, so maybe:

```
// The buffer is a list of null-terminated UTF-16 strings, followed by an
// extra L'\0' (two bytes of 0).  An empty environment must have one
// empty string, followed by an extra L'\0'.
```





Comment at: lldb/source/Host/windows/ProcessLauncherWindows.cpp:39
   buffer.push_back(0);
+  // If the environment was empty, we must insert an extra byte to ensure a
+  // double null.

"... an extra **two** bytes ..."



Comment at: lldb/source/Host/windows/ProcessLauncherWindows.cpp:41
+  // double null.
+  if (env.empty()) {
+buffer.push_back(0);

There would be no harm in always adding the extra terminator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76835



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

[Lldb-commits] [PATCH] D75275: [lldb/CMake] Use PYTHON_HOME as a hint to find Python 3.

[Adrian McCarthy via Phabricator via lldb-commits]

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

Thanks for helping figure this out!


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D75275



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

[Lldb-commits] [PATCH] D74010: Fix BroadcasterManager::RemoveListener to really remove the listener

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

Wow, cool bug.

It's too bad the original code re-used an iterator variable instead of make a 
new name (which would have helped the compiler find the problem).  Note that 
the one they used is shadowed just a couple lines later.

It's too bad the original code feels it's necessary to create iter and end_iter 
up front.  I know raw loops require this trick to avoid re-computing the end 
iterator on each iteration through the loop, but that shouldn't be necessary on 
algorithms like `find_if`.

It's too bad that erase with an end iterator isn't just a safe no-op, so that 
zillions of callers aren't required to check find's return value.  Without the 
visual noise, it would be easier to write exactly what you want.

It's too bad the compiler cannot recognize that `find_if` has no side effects 
and thus ignoring its return value makes the statement a no-op.

It's too bad the `std::erase(std::remove_if(...))` idiom is so cumbersome.  I 
realize that would likely be overkill here, since you apparently want to erase 
just the first one that matches the predicate.  Nonetheless, it would be harder 
to make this kind of bug.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74010



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

[Lldb-commits] [PATCH] D74001: Fix after c25938d

[Adrian McCarthy via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGfb0d2d455f56: Fix after c25938d (authored by amccarth).
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D74001?vs=242440=242465#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74001

Files:
  
lldb/packages/Python/lldbsuite/test/commands/add-dsym/uuid/TestAddDsymCommand.py
  lldb/source/Commands/CommandObjectTarget.cpp

Index: lldb/source/Commands/CommandObjectTarget.cpp
===
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -4119,23 +4119,6 @@
   target->GetImages().FindModules(module_spec, matching_modules);
 }
 
-if (matching_modules.IsEmpty()) {
-  StreamString ss_symfile_uuid;
-  if (module_spec.GetUUID().IsValid()) {
-ss_symfile_uuid << " (";
-module_spec.GetUUID().Dump(_symfile_uuid);
-ss_symfile_uuid << ')';
-  }
-  result.AppendErrorWithFormat(
-  "symbol file '%s'%s does not match any existing module%s\n",
-  symfile_path, ss_symfile_uuid.GetData(),
-  !llvm::sys::fs::is_regular_file(symbol_fspec.GetPath())
-  ? "\n   please specify the full path to the symbol file"
-  : "");
-  result.SetStatus(eReturnStatusFailed);
-  return false;
-}
-
 if (matching_modules.GetSize() > 1) {
   result.AppendErrorWithFormat("multiple modules match symbol file '%s', "
"use the --uuid option to resolve the "
@@ -4144,65 +4127,72 @@
   result.SetStatus(eReturnStatusFailed);
   return false;
 }
-
-assert(matching_modules.GetSize() == 1);
-ModuleSP module_sp(matching_modules.GetModuleAtIndex(0));
-
-// The module has not yet created its symbol vendor, we can just give
-// the existing target module the symfile path to use for when it
-// decides to create it!
-module_sp->SetSymbolFileFileSpec(symbol_fspec);
-
-SymbolFile *symbol_file =
-module_sp->GetSymbolFile(true, ());
-if (!symbol_file) {
-  result.AppendErrorWithFormat("symbol file '%s' could not be loaded\n",
-   symfile_path);
-  result.SetStatus(eReturnStatusFailed);
-  module_sp->SetSymbolFileFileSpec(FileSpec());
-  return false;
-}
 
-ObjectFile *object_file = symbol_file->GetObjectFile();
-if (!object_file || object_file->GetFileSpec() != symbol_fspec) {
-  result.AppendError("the object file could not be loaded\n");
-  result.SetStatus(eReturnStatusFailed);
+if (matching_modules.GetSize() == 1) {
+  ModuleSP module_sp(matching_modules.GetModuleAtIndex(0));
+
+  // The module has not yet created its symbol vendor, we can just give
+  // the existing target module the symfile path to use for when it
+  // decides to create it!
+  module_sp->SetSymbolFileFileSpec(symbol_fspec);
+
+  SymbolFile *symbol_file =
+  module_sp->GetSymbolFile(true, ());
+  if (symbol_file) {
+ObjectFile *object_file = symbol_file->GetObjectFile();
+if (object_file && object_file->GetFileSpec() == symbol_fspec) {
+  // Provide feedback that the symfile has been successfully added.
+  const FileSpec _fs = module_sp->GetFileSpec();
+  result.AppendMessageWithFormat(
+  "symbol file '%s' has been added to '%s'\n", symfile_path,
+  module_fs.GetPath().c_str());
+
+  // Let clients know something changed in the module if it is
+  // currently loaded
+  ModuleList module_list;
+  module_list.Append(module_sp);
+  target->SymbolsDidLoad(module_list);
+
+  // Make sure we load any scripting resources that may be embedded
+  // in the debug info files in case the platform supports that.
+  Status error;
+  StreamString feedback_stream;
+  module_sp->LoadScriptingResourceInTarget(target, error,
+   _stream);
+  if (error.Fail() && error.AsCString())
+result.AppendWarningWithFormat(
+"unable to load scripting data for module %s - error "
+"reported was %s",
+module_sp->GetFileSpec()
+.GetFileNameStrippingExtension()
+.GetCString(),
+error.AsCString());
+  else if (feedback_stream.GetSize())
+result.AppendWarning(feedback_stream.GetData());
+
+  flush = true;
+  result.SetStatus(eReturnStatusSuccessFinishResult);
+  return true;
+}
+  }
+  // Clear the symbol file spec if anything went wrong
   module_sp->SetSymbolFileFileSpec(FileSpec());
-  return false;
 }
-
-

[Lldb-commits] [PATCH] D74001: Fix after c25938d

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth created this revision.
amccarth added reviewers: jasonmolenda, clayborg.

My refactor caused some changes in error reporting that TestAddDsymCommand.py 
was checking, so this restores some of the changes to preserve the old behavior.

Putting this through review rather than committing directly because it's one of 
a couple alternatives discussed, and the affected test is currently XFAILed, so 
we have time to decide the best way forward.


https://reviews.llvm.org/D74001

Files:
  lldb/source/Commands/CommandObjectTarget.cpp

Index: lldb/source/Commands/CommandObjectTarget.cpp
===
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -4119,23 +4119,6 @@
   target->GetImages().FindModules(module_spec, matching_modules);
 }
 
-if (matching_modules.IsEmpty()) {
-  StreamString ss_symfile_uuid;
-  if (module_spec.GetUUID().IsValid()) {
-ss_symfile_uuid << " (";
-module_spec.GetUUID().Dump(_symfile_uuid);
-ss_symfile_uuid << ')';
-  }
-  result.AppendErrorWithFormat(
-  "symbol file '%s'%s does not match any existing module%s\n",
-  symfile_path, ss_symfile_uuid.GetData(),
-  !llvm::sys::fs::is_regular_file(symbol_fspec.GetPath())
-  ? "\n   please specify the full path to the symbol file"
-  : "");
-  result.SetStatus(eReturnStatusFailed);
-  return false;
-}
-
 if (matching_modules.GetSize() > 1) {
   result.AppendErrorWithFormat("multiple modules match symbol file '%s', "
"use the --uuid option to resolve the "
@@ -4144,65 +4127,72 @@
   result.SetStatus(eReturnStatusFailed);
   return false;
 }
-
-assert(matching_modules.GetSize() == 1);
-ModuleSP module_sp(matching_modules.GetModuleAtIndex(0));
-
-// The module has not yet created its symbol vendor, we can just give
-// the existing target module the symfile path to use for when it
-// decides to create it!
-module_sp->SetSymbolFileFileSpec(symbol_fspec);
-
-SymbolFile *symbol_file =
-module_sp->GetSymbolFile(true, ());
-if (!symbol_file) {
-  result.AppendErrorWithFormat("symbol file '%s' could not be loaded\n",
-   symfile_path);
-  result.SetStatus(eReturnStatusFailed);
-  module_sp->SetSymbolFileFileSpec(FileSpec());
-  return false;
-}
 
-ObjectFile *object_file = symbol_file->GetObjectFile();
-if (!object_file || object_file->GetFileSpec() != symbol_fspec) {
-  result.AppendError("the object file could not be loaded\n");
-  result.SetStatus(eReturnStatusFailed);
+if (matching_modules.GetSize() == 1) {
+  ModuleSP module_sp(matching_modules.GetModuleAtIndex(0));
+
+  // The module has not yet created its symbol vendor, we can just give
+  // the existing target module the symfile path to use for when it
+  // decides to create it!
+  module_sp->SetSymbolFileFileSpec(symbol_fspec);
+
+  SymbolFile *symbol_file =
+  module_sp->GetSymbolFile(true, ());
+  if (symbol_file) {
+ObjectFile *object_file = symbol_file->GetObjectFile();
+if (object_file && object_file->GetFileSpec() == symbol_fspec) {
+  // Provide feedback that the symfile has been successfully added.
+  const FileSpec _fs = module_sp->GetFileSpec();
+  result.AppendMessageWithFormat(
+  "symbol file '%s' has been added to '%s'\n", symfile_path,
+  module_fs.GetPath().c_str());
+
+  // Let clients know something changed in the module if it is
+  // currently loaded
+  ModuleList module_list;
+  module_list.Append(module_sp);
+  target->SymbolsDidLoad(module_list);
+
+  // Make sure we load any scripting resources that may be embedded
+  // in the debug info files in case the platform supports that.
+  Status error;
+  StreamString feedback_stream;
+  module_sp->LoadScriptingResourceInTarget(target, error,
+   _stream);
+  if (error.Fail() && error.AsCString())
+result.AppendWarningWithFormat(
+"unable to load scripting data for module %s - error "
+"reported was %s",
+module_sp->GetFileSpec()
+.GetFileNameStrippingExtension()
+.GetCString(),
+error.AsCString());
+  else if (feedback_stream.GetSize())
+result.AppendWarning(feedback_stream.GetData());
+
+  flush = true;
+  result.SetStatus(eReturnStatusSuccessFinishResult);
+  return true;
+}
+  }
+  // Clear the symbol file spec if anything went wrong
   module_sp->SetSymbolFileFileSpec(FileSpec());
-  return false;
 }
-
-  

[Lldb-commits] [PATCH] D73589: Improve help text for (lldb) target symbols add

[Adrian McCarthy via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0e362d82b97f: Improve help text for (lldb) target symbols 
add (authored by amccarth).
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D73589?vs=241013=242189#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73589

Files:
  lldb/source/Commands/CommandObjectTarget.cpp


Index: lldb/source/Commands/CommandObjectTarget.cpp
===
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -3999,19 +3999,20 @@
   : CommandObjectParsed(
 interpreter, "target symbols add",
 "Add a debug symbol file to one of the target's current modules by 
"
-"specifying a path to a debug symbols file, or using the options "
-"to specify a module to download symbols for.",
+"specifying a path to a debug symbols file or by using the options 
"
+"to specify a module.",
 "target symbols add  []",
 eCommandRequiresTarget),
 m_option_group(),
 m_file_option(
 LLDB_OPT_SET_1, false, "shlib", 's',
 CommandCompletions::eModuleCompletion, eArgTypeShlibName,
-"Fullpath or basename for module to find debug symbols for."),
+"Locate the debug symbols for the shared library specified by "
+"name."),
 m_current_frame_option(
 LLDB_OPT_SET_2, false, "frame", 'F',
-"Locate the debug symbols the currently selected frame.", false,
-true)
+"Locate the debug symbols for the currently selected frame.",
+false, true)
 
   {
 m_option_group.Append(_uuid_option_group, LLDB_OPT_SET_ALL,


Index: lldb/source/Commands/CommandObjectTarget.cpp
===
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -3999,19 +3999,20 @@
   : CommandObjectParsed(
 interpreter, "target symbols add",
 "Add a debug symbol file to one of the target's current modules by "
-"specifying a path to a debug symbols file, or using the options "
-"to specify a module to download symbols for.",
+"specifying a path to a debug symbols file or by using the options "
+"to specify a module.",
 "target symbols add  []",
 eCommandRequiresTarget),
 m_option_group(),
 m_file_option(
 LLDB_OPT_SET_1, false, "shlib", 's',
 CommandCompletions::eModuleCompletion, eArgTypeShlibName,
-"Fullpath or basename for module to find debug symbols for."),
+"Locate the debug symbols for the shared library specified by "
+"name."),
 m_current_frame_option(
 LLDB_OPT_SET_2, false, "frame", 'F',
-"Locate the debug symbols the currently selected frame.", false,
-true)
+"Locate the debug symbols for the currently selected frame.",
+false, true)
 
   {
 m_option_group.Append(_uuid_option_group, LLDB_OPT_SET_ALL,
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D73594: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols

[Adrian McCarthy via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc25938d57b1c: Refactor 
CommandObjectTargetSymbolsAdd::AddModuleSymbols (authored by amccarth).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73594

Files:
  lldb/source/Commands/CommandObjectTarget.cpp

Index: lldb/source/Commands/CommandObjectTarget.cpp
===
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -4053,12 +4053,10 @@
 module_spec.GetFileSpec().GetFilename() = symbol_fspec.GetFilename();
 }
 
-// We now have a module that represents a symbol file that can be used
-// for a module that might exist in the current target, so we need to
-// find that module in the target
-ModuleList matching_module_list;
+// Now module_spec represents a symbol file for a module that might exist
+// in the current target.  Let's find possible matches.
+ModuleList matching_modules;
 
-size_t num_matches = 0;
 // First extract all module specs from the symbol file
 lldb_private::ModuleSpecList symfile_module_specs;
 if (ObjectFile::GetModuleSpecifications(module_spec.GetSymbolFileSpec(),
@@ -4069,34 +4067,30 @@
   target_arch_module_spec.GetArchitecture() = target->GetArchitecture();
   if (symfile_module_specs.FindMatchingModuleSpec(target_arch_module_spec,
   symfile_module_spec)) {
-// See if it has a UUID?
 if (symfile_module_spec.GetUUID().IsValid()) {
   // It has a UUID, look for this UUID in the target modules
   ModuleSpec symfile_uuid_module_spec;
   symfile_uuid_module_spec.GetUUID() = symfile_module_spec.GetUUID();
   target->GetImages().FindModules(symfile_uuid_module_spec,
-  matching_module_list);
-  num_matches = matching_module_list.GetSize();
+  matching_modules);
 }
   }
 
-  if (num_matches == 0) {
-// No matches yet, iterate through the module specs to find a UUID
-// value that we can match up to an image in our target
-const size_t num_symfile_module_specs =
-symfile_module_specs.GetSize();
-for (size_t i = 0; i < num_symfile_module_specs && num_matches == 0;
-  ++i) {
+  if (matching_modules.IsEmpty()) {
+// No matches yet.  Iterate through the module specs to find a UUID
+// value that we can match up to an image in our target.
+const size_t num_symfile_module_specs = symfile_module_specs.GetSize();
+for (size_t i = 0;
+ i < num_symfile_module_specs && matching_modules.IsEmpty(); ++i) {
   if (symfile_module_specs.GetModuleSpecAtIndex(
   i, symfile_module_spec)) {
 if (symfile_module_spec.GetUUID().IsValid()) {
-  // It has a UUID, look for this UUID in the target modules
+  // It has a UUID.  Look for this UUID in the target modules.
   ModuleSpec symfile_uuid_module_spec;
   symfile_uuid_module_spec.GetUUID() =
   symfile_module_spec.GetUUID();
   target->GetImages().FindModules(symfile_uuid_module_spec,
-  matching_module_list);
-  num_matches = matching_module_list.GetSize();
+  matching_modules);
 }
   }
 }
@@ -4104,13 +4098,11 @@
 }
 
 // Just try to match up the file by basename if we have no matches at
-// this point
-if (num_matches == 0) {
-  target->GetImages().FindModules(module_spec, matching_module_list);
-  num_matches = matching_module_list.GetSize();
-}
+// this point.  For example, module foo might have symbols in foo.debug.
+if (matching_modules.IsEmpty())
+  target->GetImages().FindModules(module_spec, matching_modules);
 
-while (num_matches == 0) {
+while (matching_modules.IsEmpty()) {
   ConstString filename_no_extension(
   module_spec.GetFileSpec().GetFileNameStrippingExtension());
   // Empty string returned, let's bail
@@ -4123,82 +4115,93 @@
 
   // Replace basename with one fewer extension
   module_spec.GetFileSpec().GetFilename() = filename_no_extension;
-  target->GetImages().FindModules(module_spec, matching_module_list);
-  num_matches = matching_module_list.GetSize();
+  target->GetImages().FindModules(module_spec, matching_modules);
+}
+
+if (matching_modules.IsEmpty()) {
+  StreamString ss_symfile_uuid;
+  if (module_spec.GetUUID().IsValid()) {
+ss_symfile_uuid << " (";
+module_spec.GetUUID().Dump(_symfile_uuid);
+

[Lldb-commits] [PATCH] D73589: Improve help text for (lldb) target symbols add

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

This is low risk and it did get positive comments from one non-reviewer, so I'm 
going to land this.


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

https://reviews.llvm.org/D73589



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

[Lldb-commits] [PATCH] D73589: Improve help text for (lldb) target symbols add

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

In D73589#1845971 , @jasonmolenda 
wrote:

> Looks good, for the --shlib option I would get rid of the "full path or base 
> name" language and just say "name", my two cents.


I agree just "name" seems fine.  Done.


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

https://reviews.llvm.org/D73589



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

[Lldb-commits] [PATCH] D73594: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols

[Adrian McCarthy via Phabricator via lldb-commits]

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



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4175
+
+if (object_file->GetFileSpec() != symbol_fspec) {
+  result.AppendWarning("there is a discrepancy between the module file "

clayborg wrote:
> amccarth wrote:
> > amccarth wrote:
> > > clayborg wrote:
> > > > labath wrote:
> > > > > This part is not good. Everywhere else except pdbs this means that we 
> > > > > were in fact unable to load the symbol file. What happens is that if 
> > > > > we cannot load the object file representing the symbol file (at [[ 
> > > > > https://github.com/llvm/llvm-project/blob/master/lldb/source/Symbol/SymbolVendor.cpp#L48
> > > > >  | SymbolVendor.cpp:48 ]]), we fall back to creating a SymbolFile 
> > > > > using the object file of the original module (line 52).
> > > > > 
> > > > > The effect of that is that the symbol file creation will always 
> > > > > succeed, the previous checks are more-or-less useless, and the only 
> > > > > way to check if we are really using the symbols from the file the 
> > > > > user specified is to compare the file specs.
> > > > > 
> > > > > Now, PDB symbol files are abusing this fallback, and reading the 
> > > > > symbols from the pdb files even though they were in fact asked to 
> > > > > read them from the executable file. This is why this may sound like a 
> > > > > "discrepancy" coming from the pdb world, but everywhere else this 
> > > > > just means that the symbol file was not actually loaded.
> > > > This could also fail if the symbol file spec was a bundle which got 
> > > > resolved when the module was loaded. If the user specified 
> > > > "/path/to/foo.dSYM" and the underlying code was able to resolve the 
> > > > symbol file in the dSYM bundle to 
> > > > "/path/to/foo.dSYM/Contents/Resources/DWARF/foo".
> > > Interesting.  I did not expect that fallback behavior from the API.
> > > 
> > > > PDB symbol files are abusing this fallback
> > > 
> > > I'm not sure there's abuse going on here.  I'm not understanding 
> > > something about how lldb models this situation.  Consider the case where 
> > > the user explicitly asks lldb to load symbols from a PDB:
> > > 
> > > (lldb) target add symbols D:\my_symbols\foo.pdb
> > > 
> > > Then `symbol_file` is the PDB and `object_file` is the executable or DLL. 
> > >  I would expect those file specs to match only in the case where the 
> > > symbols are in the binary (in other words, when symbol file and object 
> > > file are the same file).  Even ignoring the PDB case, it seems this 
> > > wouldn't even work in the case you mentioned above where the object file 
> > > is stripped and the symbol file is an unstripped copy of the object file 
> > > (possibly with a different name).  You can also get here if the symbols 
> > > were matched by UUID rather than file spec.  Or when the symbols were 
> > > matched by the basename minus some extension.
> > > 
> > > Given that dsyms work, I must be misunderstanding something here.  Can 
> > > you help me understand?
> > > 
> > > What's the right thing to do here?  If I just undo this refactor, then 
> > > we're back to silent failures when the symbols exist in a different file 
> > > than the object.
> > In your example, the file specs do not match.
> > 
> > 1. The old code would therefore reject the symbol file and return an error.
> > 2. The new code would therefore emit a warning but proceed with the 
> > selected symbol file and return success.
> > 
> > Do you want either of these behaviors or something else?
> all I know is I can and have typed:
> ```
> (lldb) target symbols add /path/to/foo.dSYM
> ```
> And it would successfully associate it with the binary, probably because the 
> code on 4071 would see a UUID and accept it before it gets to this point. 
> 
> What is the point of the warning? When would this happen?
There are examples in the thread with Pavel just below this thread.  On 
Windows, your symbols may come from a file called `foo.pdb` for an executable 
called `foo.exe`.  The old code would successfully load the PDB file, then 
decide it was no good because the file specs don't match.  (Heck, at this 
point, we've just gone through a bunch of gyrations to allow matching of 
differing file specs.)  So you'd get a mostly silent failure for code that 
actually worked.

My intent was to simply warn about such cases without actually forcing a 
failure, and to do so with some explicit feedback so that the user understands.

I'll wait to hear what Pavel says.  If necessary, I'll just roll back this part 
of the change (so that this remains just a refactor), and I'll figure out a way 
around the problem in a later patch.


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

https://reviews.llvm.org/D73594



___
lldb-commits mailing list
lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D73594: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols

[Adrian McCarthy via Phabricator via lldb-commits]

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



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4175
+
+if (object_file->GetFileSpec() != symbol_fspec) {
+  result.AppendWarning("there is a discrepancy between the module file "

amccarth wrote:
> clayborg wrote:
> > labath wrote:
> > > This part is not good. Everywhere else except pdbs this means that we 
> > > were in fact unable to load the symbol file. What happens is that if we 
> > > cannot load the object file representing the symbol file (at [[ 
> > > https://github.com/llvm/llvm-project/blob/master/lldb/source/Symbol/SymbolVendor.cpp#L48
> > >  | SymbolVendor.cpp:48 ]]), we fall back to creating a SymbolFile using 
> > > the object file of the original module (line 52).
> > > 
> > > The effect of that is that the symbol file creation will always succeed, 
> > > the previous checks are more-or-less useless, and the only way to check 
> > > if we are really using the symbols from the file the user specified is to 
> > > compare the file specs.
> > > 
> > > Now, PDB symbol files are abusing this fallback, and reading the symbols 
> > > from the pdb files even though they were in fact asked to read them from 
> > > the executable file. This is why this may sound like a "discrepancy" 
> > > coming from the pdb world, but everywhere else this just means that the 
> > > symbol file was not actually loaded.
> > This could also fail if the symbol file spec was a bundle which got 
> > resolved when the module was loaded. If the user specified 
> > "/path/to/foo.dSYM" and the underlying code was able to resolve the symbol 
> > file in the dSYM bundle to "/path/to/foo.dSYM/Contents/Resources/DWARF/foo".
> Interesting.  I did not expect that fallback behavior from the API.
> 
> > PDB symbol files are abusing this fallback
> 
> I'm not sure there's abuse going on here.  I'm not understanding something 
> about how lldb models this situation.  Consider the case where the user 
> explicitly asks lldb to load symbols from a PDB:
> 
> (lldb) target add symbols D:\my_symbols\foo.pdb
> 
> Then `symbol_file` is the PDB and `object_file` is the executable or DLL.  I 
> would expect those file specs to match only in the case where the symbols are 
> in the binary (in other words, when symbol file and object file are the same 
> file).  Even ignoring the PDB case, it seems this wouldn't even work in the 
> case you mentioned above where the object file is stripped and the symbol 
> file is an unstripped copy of the object file (possibly with a different 
> name).  You can also get here if the symbols were matched by UUID rather than 
> file spec.  Or when the symbols were matched by the basename minus some 
> extension.
> 
> Given that dsyms work, I must be misunderstanding something here.  Can you 
> help me understand?
> 
> What's the right thing to do here?  If I just undo this refactor, then we're 
> back to silent failures when the symbols exist in a different file than the 
> object.
In your example, the file specs do not match.

1. The old code would therefore reject the symbol file and return an error.
2. The new code would therefore emit a warning but proceed with the selected 
symbol file and return success.

Do you want either of these behaviors or something else?


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

https://reviews.llvm.org/D73594



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

[Lldb-commits] [PATCH] D73594: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols

[Adrian McCarthy via Phabricator via lldb-commits]

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



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4149
+
+lldbassert(matching_modules.GetSize() == 1);
+ModuleSP module_sp(matching_modules.GetModuleAtIndex(0));

clayborg wrote:
> labath wrote:
> > This should be a regular assert according to 
> > .
> This assert will never trigger with the above code already checking for empty 
> on line 4123 and then checking if size > 1 on line 4140. Remove it?
It's true that this assert won't happen given the early returns above, so I 
will remove it if you like.

But I did it intentionally:  (1) to document the requirement and (2) to protect 
against future changes accidentally breaking the assumptions.  If the function 
were much shorter, then it might be obvious enough as is.  But early returns, 
especially halfway down a long function, can be easy to miss.


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

https://reviews.llvm.org/D73594



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

[Lldb-commits] [PATCH] D73594: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth marked 3 inline comments as done.
amccarth added a comment.

Thanks for the feedback.  Obviously I'm confused about how LLDB handles split 
debug info, so I need more clarification about how to proceed.




Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4103
+// this point.
+// TODO:  Is this part worthwhile?  `foo.exe` will never match `foo.pdb`
+if (matching_modules.IsEmpty())

labath wrote:
> This is not unreasonable in the non-pdb world. You can have a stripped 
> version of a file somewhere prepared for deployment to some device (or 
> downloaded from the device), and then you'll also have an unstripped version 
> of that file (with the same name) in some build folder.
Sure, if you've got two files with the same name in two directories that's fine.

But the loop tries peeling the extension off of the supplied file name and 
comparing it to the target's file name.  I guess it's trying to match something 
like `foo.unstripped` to `foo`.  Is that a typical thing?



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4175-4178
+if (object_file->GetFileSpec() != symbol_fspec) {
+  result.AppendWarning("there is a discrepancy between the module file "
+   "spec and the symbol file spec\n");
+}

clayborg wrote:
> labath wrote:
> > This part is not good. Everywhere else except pdbs this means that we were 
> > in fact unable to load the symbol file. What happens is that if we cannot 
> > load the object file representing the symbol file (at [[ 
> > https://github.com/llvm/llvm-project/blob/master/lldb/source/Symbol/SymbolVendor.cpp#L48
> >  | SymbolVendor.cpp:48 ]]), we fall back to creating a SymbolFile using the 
> > object file of the original module (line 52).
> > 
> > The effect of that is that the symbol file creation will always succeed, 
> > the previous checks are more-or-less useless, and the only way to check if 
> > we are really using the symbols from the file the user specified is to 
> > compare the file specs.
> > 
> > Now, PDB symbol files are abusing this fallback, and reading the symbols 
> > from the pdb files even though they were in fact asked to read them from 
> > the executable file. This is why this may sound like a "discrepancy" coming 
> > from the pdb world, but everywhere else this just means that the symbol 
> > file was not actually loaded.
> This could also fail if the symbol file spec was a bundle which got resolved 
> when the module was loaded. If the user specified "/path/to/foo.dSYM" and the 
> underlying code was able to resolve the symbol file in the dSYM bundle to 
> "/path/to/foo.dSYM/Contents/Resources/DWARF/foo".
Interesting.  I did not expect that fallback behavior from the API.

> PDB symbol files are abusing this fallback

I'm not sure there's abuse going on here.  I'm not understanding something 
about how lldb models this situation.  Consider the case where the user 
explicitly asks lldb to load symbols from a PDB:

(lldb) target add symbols D:\my_symbols\foo.pdb

Then `symbol_file` is the PDB and `object_file` is the executable or DLL.  I 
would expect those file specs to match only in the case where the symbols are 
in the binary (in other words, when symbol file and object file are the same 
file).  Even ignoring the PDB case, it seems this wouldn't even work in the 
case you mentioned above where the object file is stripped and the symbol file 
is an unstripped copy of the object file (possibly with a different name).  You 
can also get here if the symbols were matched by UUID rather than file spec.  
Or when the symbols were matched by the basename minus some extension.

Given that dsyms work, I must be misunderstanding something here.  Can you help 
me understand?

What's the right thing to do here?  If I just undo this refactor, then we're 
back to silent failures when the symbols exist in a different file than the 
object.


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

https://reviews.llvm.org/D73594



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

[Lldb-commits] [PATCH] D73594: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth created this revision.
amccarth added reviewers: clayborg, jasonmolenda.

- [NFC] Renamed local `matching_module_list` to `matching_modules` for 
conciseness.

- [NFC] Eliminated redundant local variable `num_matches` to reduce the risk 
that changes get it out of sync with `matching_modules.GetSize()`.

- Used an early return from case where the symbol file specified matches 
multiple modules.  This is a slight behavior change, but it's an improvement: 
It didn't make sense to tell the user that the symbol file simultaneously 
matched multiple modules and no modules.

- [NFC] Used an early return from the case where no matches are found, to 
better align with LLVM coding style.

- [NFC] Simplified call of `AppendWarningWithFormat("%s", stuff)` to 
`AppendWarning(stuff)`.  I don't think this adds any copies.  It does construct 
a StringRef, but it was going to have to scan the string for the length anyway.

- [NFC] Removed unnecessary comments and reworded others for clarity.

- Used an early return if the symbol file could not be loaded.  This is a 
behavior change because previously it could fail silently.

- Used an early return if the object file could not be retrieved from the 
symbol file.  Again, this is a change because now there's an error message.

- If we successfully load a symbol file that seems to match, but then detect a 
file name mismatch, now we only issue a warning.  We've already determined they 
match, so it seems pointless to change our minds.

- [NFC] Eliminated a namespace alias that wasn't particularly helpful.


https://reviews.llvm.org/D73594

Files:
  lldb/source/Commands/CommandObjectTarget.cpp

Index: lldb/source/Commands/CommandObjectTarget.cpp
===
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -4054,12 +4054,10 @@
 module_spec.GetFileSpec().GetFilename() = symbol_fspec.GetFilename();
 }
 
-// We now have a module that represents a symbol file that can be used
-// for a module that might exist in the current target, so we need to
-// find that module in the target
-ModuleList matching_module_list;
+// Now module_spec represents a symbol file for a module that might exist
+// in the current target.  Let's find possible matches.
+ModuleList matching_modules;
 
-size_t num_matches = 0;
 // First extract all module specs from the symbol file
 lldb_private::ModuleSpecList symfile_module_specs;
 if (ObjectFile::GetModuleSpecifications(module_spec.GetSymbolFileSpec(),
@@ -4070,34 +4068,30 @@
   target_arch_module_spec.GetArchitecture() = target->GetArchitecture();
   if (symfile_module_specs.FindMatchingModuleSpec(target_arch_module_spec,
   symfile_module_spec)) {
-// See if it has a UUID?
 if (symfile_module_spec.GetUUID().IsValid()) {
   // It has a UUID, look for this UUID in the target modules
   ModuleSpec symfile_uuid_module_spec;
   symfile_uuid_module_spec.GetUUID() = symfile_module_spec.GetUUID();
   target->GetImages().FindModules(symfile_uuid_module_spec,
-  matching_module_list);
-  num_matches = matching_module_list.GetSize();
+  matching_modules);
 }
   }
 
-  if (num_matches == 0) {
-// No matches yet, iterate through the module specs to find a UUID
-// value that we can match up to an image in our target
-const size_t num_symfile_module_specs =
-symfile_module_specs.GetSize();
-for (size_t i = 0; i < num_symfile_module_specs && num_matches == 0;
-  ++i) {
+  if (matching_modules.IsEmpty()) {
+// No matches yet.  Iterate through the module specs to find a UUID
+// value that we can match up to an image in our target.
+const size_t num_symfile_module_specs = symfile_module_specs.GetSize();
+for (size_t i = 0;
+ i < num_symfile_module_specs && matching_modules.IsEmpty(); ++i) {
   if (symfile_module_specs.GetModuleSpecAtIndex(
   i, symfile_module_spec)) {
 if (symfile_module_spec.GetUUID().IsValid()) {
-  // It has a UUID, look for this UUID in the target modules
+  // It has a UUID.  Look for this UUID in the target modules.
   ModuleSpec symfile_uuid_module_spec;
   symfile_uuid_module_spec.GetUUID() =
   symfile_module_spec.GetUUID();
   target->GetImages().FindModules(symfile_uuid_module_spec,
-  matching_module_list);
-  num_matches = matching_module_list.GetSize();
+  matching_modules);
 }
   }
 }
@@ -4105,13 +4099,12 @@
 }
 
 

[Lldb-commits] [PATCH] D73589: Improve help text for (lldb) target symbols add

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth created this revision.
amccarth added a reviewer: aaron.ballman.

There were some missing words and awkward syntax.  I think this is clearer.


https://reviews.llvm.org/D73589

Files:
  lldb/source/Commands/CommandObjectTarget.cpp


Index: lldb/source/Commands/CommandObjectTarget.cpp
===
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -3999,19 +3999,20 @@
   : CommandObjectParsed(
 interpreter, "target symbols add",
 "Add a debug symbol file to one of the target's current modules by 
"
-"specifying a path to a debug symbols file, or using the options "
-"to specify a module to download symbols for.",
+"specifying a path to a debug symbols file or by using the options 
"
+"to specify a module.",
 "target symbols add  []",
 eCommandRequiresTarget),
 m_option_group(),
 m_file_option(
 LLDB_OPT_SET_1, false, "shlib", 's',
 CommandCompletions::eModuleCompletion, eArgTypeShlibName,
-"Fullpath or basename for module to find debug symbols for."),
+"Locate the debug symbols for the shared library specified by "
+"full path or base name."),
 m_current_frame_option(
 LLDB_OPT_SET_2, false, "frame", 'F',
-"Locate the debug symbols the currently selected frame.", false,
-true)
+"Locate the debug symbols for the currently selected frame.",
+false, true)
 
   {
 m_option_group.Append(_uuid_option_group, LLDB_OPT_SET_ALL,


Index: lldb/source/Commands/CommandObjectTarget.cpp
===
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -3999,19 +3999,20 @@
   : CommandObjectParsed(
 interpreter, "target symbols add",
 "Add a debug symbol file to one of the target's current modules by "
-"specifying a path to a debug symbols file, or using the options "
-"to specify a module to download symbols for.",
+"specifying a path to a debug symbols file or by using the options "
+"to specify a module.",
 "target symbols add  []",
 eCommandRequiresTarget),
 m_option_group(),
 m_file_option(
 LLDB_OPT_SET_1, false, "shlib", 's',
 CommandCompletions::eModuleCompletion, eArgTypeShlibName,
-"Fullpath or basename for module to find debug symbols for."),
+"Locate the debug symbols for the shared library specified by "
+"full path or base name."),
 m_current_frame_option(
 LLDB_OPT_SET_2, false, "frame", 'F',
-"Locate the debug symbols the currently selected frame.", false,
-true)
+"Locate the debug symbols for the currently selected frame.",
+false, true)
 
   {
 m_option_group.Append(_uuid_option_group, LLDB_OPT_SET_ALL,
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70458: [NFC] Refactor and improve comments in CommandObjectTarget

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth closed this revision.
amccarth added a comment.

This landed in 3b69f0c5550a229dd6d39e361182cdd7cecc36a4 
, but 
there was a typo in the patch description so the tools didn't automatically 
close this.


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

https://reviews.llvm.org/D70458



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

[Lldb-commits] [PATCH] D70840: [LLDB] [DWARF] Strip out the thumb bit from addresses on ARM

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth resigned from this revision.
amccarth added a comment.

I'm following along, but I don't think I have enough domain knowledge to 
qualify as a reviewer.


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

https://reviews.llvm.org/D70840



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

[Lldb-commits] [PATCH] D70778: [LLDB] [PECOFF] Factorize mapping section names to types using StringSwitch. NFCI.

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:864-866
+// If the StringSwitch above picked any type, including
+// eSectionTypeOther, accept that instead of the generic mappings
+// based on flags below.

labath wrote:
> This makes pretty weird control flow. I think it would be way clearer if all 
> of this code were moved into a function like `GetSectionType` (there's a 
> function like that in ObjectFileELF). Then you can use return statements to 
> shortcut control flow, like so:
> ```
> if (m_sect_headers[idx].flags & llvm::COFF::IMAGE_SCN_CNT_CODE &&
>   ((const_sect_name == g_code_sect_name) ||
>(const_sect_name == g_CODE_sect_name)))
>   return eSectionTypeCode;
> if (...)
>   return eSectionTypeZeroFill;
> SectionType type = StringSwitch(name)...;
> if (type != Invalid)
>   return type;
> ...
> ```
+1.  Factoring the decision tree into a separate function is part of what I was 
trying to ask for in the patch.  That it simplifies things with early returns 
is an added bonus.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70778



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

[Lldb-commits] [PATCH] D70745: [LLDB] [PECOFF] Look for the truncated ".eh_fram" section name

[Adrian McCarthy via Phabricator via lldb-commits]

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

Nice.  LGTM.


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

https://reviews.llvm.org/D70745



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

[Lldb-commits] [PATCH] D70745: [LLDB] [PECOFF] Look for the truncated ".eh_fram" section name

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

Is `.eh_frame` the only one that matters?  Should this be more general and 
compare `const_sect_name` to the full name and the truncated name for any known 
section names?

If the giant cascade of else-if were factored into a separate function, then a 
trivial unit test could verify that it returns the right section_type for a 
variety of full and truncated names (as well as other combinations of section 
headers).

Although not directly relevant to this patch, it seems like a lot this cascade 
could be made more readable and maintainable by replacing the simple 
if-name-set-section_type cases were replaced with a lookup into a table (and 
that lookup could be the one place to check both the full name and the 
truncated name).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70745



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

[Lldb-commits] [PATCH] D70742: [LLDB] [Windows] Avoid using InitializeContext for allocating a CONTEXT. NFC.

[Adrian McCarthy via Phabricator via lldb-commits]

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

That's fair.  LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70742



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

[Lldb-commits] [PATCH] D70742: [LLDB] [Windows] Avoid using InitializeContext for allocating a CONTEXT. NFC.

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

I'm good with the change, but have a couple small requests.  I hope to hear 
from others, too, as this area is outside my wheelhouse.




Comment at: 
lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp:158
+  memset(_context, 0, sizeof(m_context));
+  m_context.ContextFlags = kWinContextFlags;
   if (::SuspendThread(

As far as I understand what InitializeContext does, this seems a suitable 
replacement for how it's used here.

But if someone were to change the flag to include CONTEXT_XSTATE, then this 
would no longer work, because the xstate is variable length.

I would suggest:

1.  Eliminate the kWinContextFlags (defined at the top of this file) and just 
use CONTEXT_ALL here.  The extra name doesn't seem to clarify anything, and the 
distance between the definition and usage can make it harder for folks to 
understand this code.

2.  Add a comment saying this is a substitute for InitializeContext for Wine, 
which makes searching the code for `InitializeContext` useful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70742



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

[Lldb-commits] [PATCH] D70458: [NFC] Refactor and improve comments in CommandObjectTarget

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth updated this revision to Diff 230303.
amccarth marked an inline comment as done.
amccarth added a comment.

Reverted unintended change caught by reviewer.


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

https://reviews.llvm.org/D70458

Files:
  lldb/source/Commands/CommandObjectTarget.cpp


Index: lldb/source/Commands/CommandObjectTarget.cpp
===
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -4044,7 +4044,13 @@
   bool AddModuleSymbols(Target *target, ModuleSpec _spec, bool ,
 CommandReturnObject ) {
 const FileSpec _fspec = module_spec.GetSymbolFileSpec();
-if (symbol_fspec) {
+if (!symbol_fspec) {
+  result.AppendError(
+  "one or more executable image paths must be specified");
+  result.SetStatus(eReturnStatusFailed);
+  return false;
+}
+
 char symfile_path[PATH_MAX];
 symbol_fspec.GetPath(symfile_path, sizeof(symfile_path));
 
@@ -4052,6 +4058,7 @@
   if (!module_spec.GetFileSpec() && !module_spec.GetPlatformFileSpec())
 module_spec.GetFileSpec().GetFilename() = symbol_fspec.GetFilename();
 }
+
 // We now have a module that represents a symbol file that can be used
 // for a module that might exist in the current target, so we need to
 // find that module in the target
@@ -4112,18 +4119,16 @@
 while (num_matches == 0) {
   ConstString filename_no_extension(
   module_spec.GetFileSpec().GetFileNameStrippingExtension());
-// Empty string returned, lets bail
+  // Empty string returned, let's bail
   if (!filename_no_extension)
 break;
 
-// Check if there was no extension to strip and the basename is the
-// same
+  // Check if there was no extension to strip and the basename is the same
   if (filename_no_extension == module_spec.GetFileSpec().GetFilename())
 break;
 
-// Replace basename with one less extension
+  // Replace basename with one fewer extension
   module_spec.GetFileSpec().GetFilename() = filename_no_extension;
-
   target->GetImages().FindModules(module_spec, matching_module_list);
   num_matches = matching_module_list.GetSize();
 }
@@ -4186,27 +4191,18 @@
 }
 
 namespace fs = llvm::sys::fs;
-  if (module_spec.GetUUID().IsValid()) {
 StreamString ss_symfile_uuid;
+if (module_spec.GetUUID().IsValid()) {
+  ss_symfile_uuid << " (";
   module_spec.GetUUID().Dump(_symfile_uuid);
+  ss_symfile_uuid << ')';
+}
 result.AppendErrorWithFormat(
-"symbol file '%s' (%s) does not match any existing module%s\n",
+"symbol file '%s'%s does not match any existing module%s\n",
 symfile_path, ss_symfile_uuid.GetData(),
 !fs::is_regular_file(symbol_fspec.GetPath())
 ? "\n   please specify the full path to the symbol file"
 : "");
-  } else {
-result.AppendErrorWithFormat(
-"symbol file '%s' does not match any existing module%s\n",
-symfile_path,
-!fs::is_regular_file(symbol_fspec.GetPath())
-? "\n   please specify the full path to the symbol file"
-: "");
-  }
-} else {
-  result.AppendError(
-  "one or more executable image paths must be specified");
-}
 result.SetStatus(eReturnStatusFailed);
 return false;
   }


Index: lldb/source/Commands/CommandObjectTarget.cpp
===
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -4044,7 +4044,13 @@
   bool AddModuleSymbols(Target *target, ModuleSpec _spec, bool ,
 CommandReturnObject ) {
 const FileSpec _fspec = module_spec.GetSymbolFileSpec();
-if (symbol_fspec) {
+if (!symbol_fspec) {
+  result.AppendError(
+  "one or more executable image paths must be specified");
+  result.SetStatus(eReturnStatusFailed);
+  return false;
+}
+
 char symfile_path[PATH_MAX];
 symbol_fspec.GetPath(symfile_path, sizeof(symfile_path));
 
@@ -4052,6 +4058,7 @@
   if (!module_spec.GetFileSpec() && !module_spec.GetPlatformFileSpec())
 module_spec.GetFileSpec().GetFilename() = symbol_fspec.GetFilename();
 }
+
 // We now have a module that represents a symbol file that can be used
 // for a module that might exist in the current target, so we need to
 // find that module in the target
@@ -4112,18 +4119,16 @@
 while (num_matches == 0) {
   ConstString filename_no_extension(
   module_spec.GetFileSpec().GetFileNameStrippingExtension());
-// Empty string returned, lets bail
+  // Empty string returned, let's bail
   if (!filename_no_extension)
 break;
 
-// Check if there was no 

[Lldb-commits] [PATCH] D70458: [NFC] Refactor and improve comments in CommandObjectTarget

[Adrian McCarthy via Phabricator via lldb-commits]

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



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4059
   if (!module_spec.GetFileSpec() && !module_spec.GetPlatformFileSpec())
-  module_spec.GetFileSpec().GetFilename() = symbol_fspec.GetFilename();
+module_spec.GetFileSpec() = symbol_fspec;
 }

labath wrote:
> This does change behavior because previously the symbol file directory wasn't 
> being copied. I think that was intentional because the comment on line 4112 
> says "match up the file by basename" (and it also makes sense because if 
> you're adding symbols in an external file, then the main module file cannot 
> be the exact same path as the symbol file).
Oops.  Thanks for catching that.

ModuleSpec seems weird:  It exposes an internal members to be tweaked in 
arbitrary ways.  I would have expected that it would have to react to certain 
kinds of changes to keep itself consistent.  If it has no invariants to 
enforce, it could have been a plain struct with a bunch of public member 
variables.


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

https://reviews.llvm.org/D70458



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

[Lldb-commits] [PATCH] D70458: [NFC] Refactor and improve comments in CommandObjectTarget

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth created this revision.
amccarth added a reviewer: labath.

Made small improvements while debugging through 
CommandObjectTarget::AddModuleSymbols.

1. Refactored error case for an early out, reducing the indentation of the rest 
of this long function.
2. Clarified some comments by correcting spelling and punctuation.
3. Reduced duplicate code at the end of the function.

Tested with `ninja check-lldb`


https://reviews.llvm.org/D70458

Files:
  lldb/source/Commands/CommandObjectTarget.cpp


Index: lldb/source/Commands/CommandObjectTarget.cpp
===
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -4044,14 +4044,21 @@
   bool AddModuleSymbols(Target *target, ModuleSpec _spec, bool ,
 CommandReturnObject ) {
 const FileSpec _fspec = module_spec.GetSymbolFileSpec();
-if (symbol_fspec) {
+if (!symbol_fspec) {
+  result.AppendError(
+  "one or more executable image paths must be specified");
+  result.SetStatus(eReturnStatusFailed);
+  return false;
+}
+
 char symfile_path[PATH_MAX];
 symbol_fspec.GetPath(symfile_path, sizeof(symfile_path));
 
 if (!module_spec.GetUUID().IsValid()) {
   if (!module_spec.GetFileSpec() && !module_spec.GetPlatformFileSpec())
-  module_spec.GetFileSpec().GetFilename() = symbol_fspec.GetFilename();
+module_spec.GetFileSpec() = symbol_fspec;
 }
+
 // We now have a module that represents a symbol file that can be used
 // for a module that might exist in the current target, so we need to
 // find that module in the target
@@ -4112,18 +4119,16 @@
 while (num_matches == 0) {
   ConstString filename_no_extension(
   module_spec.GetFileSpec().GetFileNameStrippingExtension());
-// Empty string returned, lets bail
+  // Empty string returned, let's bail
   if (!filename_no_extension)
 break;
 
-// Check if there was no extension to strip and the basename is the
-// same
+  // Check if there was no extension to strip and the basename is the same
   if (filename_no_extension == module_spec.GetFileSpec().GetFilename())
 break;
 
-// Replace basename with one less extension
+  // Replace basename with one fewer extension
   module_spec.GetFileSpec().GetFilename() = filename_no_extension;
-
   target->GetImages().FindModules(module_spec, matching_module_list);
   num_matches = matching_module_list.GetSize();
 }
@@ -4186,27 +4191,18 @@
 }
 
 namespace fs = llvm::sys::fs;
-  if (module_spec.GetUUID().IsValid()) {
 StreamString ss_symfile_uuid;
+if (module_spec.GetUUID().IsValid()) {
+  ss_symfile_uuid << " (";
   module_spec.GetUUID().Dump(_symfile_uuid);
+  ss_symfile_uuid << ')';
+}
 result.AppendErrorWithFormat(
-"symbol file '%s' (%s) does not match any existing module%s\n",
+"symbol file '%s'%s does not match any existing module%s\n",
 symfile_path, ss_symfile_uuid.GetData(),
 !fs::is_regular_file(symbol_fspec.GetPath())
 ? "\n   please specify the full path to the symbol file"
 : "");
-  } else {
-result.AppendErrorWithFormat(
-"symbol file '%s' does not match any existing module%s\n",
-symfile_path,
-!fs::is_regular_file(symbol_fspec.GetPath())
-? "\n   please specify the full path to the symbol file"
-: "");
-  }
-} else {
-  result.AppendError(
-  "one or more executable image paths must be specified");
-}
 result.SetStatus(eReturnStatusFailed);
 return false;
   }


Index: lldb/source/Commands/CommandObjectTarget.cpp
===
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -4044,14 +4044,21 @@
   bool AddModuleSymbols(Target *target, ModuleSpec _spec, bool ,
 CommandReturnObject ) {
 const FileSpec _fspec = module_spec.GetSymbolFileSpec();
-if (symbol_fspec) {
+if (!symbol_fspec) {
+  result.AppendError(
+  "one or more executable image paths must be specified");
+  result.SetStatus(eReturnStatusFailed);
+  return false;
+}
+
 char symfile_path[PATH_MAX];
 symbol_fspec.GetPath(symfile_path, sizeof(symfile_path));
 
 if (!module_spec.GetUUID().IsValid()) {
   if (!module_spec.GetFileSpec() && !module_spec.GetPlatformFileSpec())
-  module_spec.GetFileSpec().GetFilename() = symbol_fspec.GetFilename();
+module_spec.GetFileSpec() = symbol_fspec;
 }
+
 // We now have a module that represents a symbol file that can be used
 // for a module that might exist in the current target, so we need to
 // find that module in the target
@@ -4112,18 

[Lldb-commits] [PATCH] D70387: [LLDB] [Windows] Allow making subprocesses use the debugger's console with LLDB_INHERIT_CONSOLE=true

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

labath has added great context here, and I generally agree with clayborg's 
ideal of optimal behavior.  That said, if memory serves, getting that behavior 
on Windows can be quite challenging, so I'm not sure if it's worth the effort.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D70387



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

[Lldb-commits] [PATCH] D70281: Fix -Wunused-result warnings in LLDB

[Adrian McCarthy via Phabricator via lldb-commits]

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

LGTM.

It's too bad that pattern is repeated three times, including the explanatory 
comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70281



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

[Lldb-commits] [PATCH] D69503: [LLDB] [Windows] Don't crash if the debugged process is unable to start up

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69503



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

[Lldb-commits] [PATCH] D69503: [LLDB] [Windows] Don't crash if the debugged process is unable to start up

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth requested changes to this revision.
amccarth added a comment.
This revision now requires changes to proceed.

Thanks for identifying this race condition.

After some poking around, I think there's a cleaner way to address it.

First, `m_session_state`'s lifetime is currently managed mostly by 
`ProcessDebugger` (base class) but for one exception in `ProcessWindows` 
(derived class).  There doesn't seem to be a good reason for that 
inconsistency, so my first step was to eliminate it and make `ProcessDebugger` 
responsible for its lifetime in all cases. This is done by moving the 
`m_session_state.reset()` to `ProcessDebugger::OnExitProcess` and having 
`ProcessWindows::OnExitProcess` call the other.  This has the nice additional 
benefit of eliminating some duplicate code and comments.

But I'm not sure we need to clear `m_session_state` even in 
`ProcessDebugger::OnExitProcess`.  There are two places where `m_session_state` 
is created:  `ProcessDebugger::AttachProcess` and 
`ProcessDebugger::LaunchProcess`.  We could put `m_session_state.reset()`s in 
those functions, where it handles failure of `WaitForDebuggerConnection`.  But 
I'm not even sure if _that_ is necessary.

By adding a virtual destructor to the base class, it seems everything cleans up 
naturally before starting the next debugging session.

Here's what it would look like:  https://reviews.llvm.org/P8168

(If I'm wrong, and it _is_ important for us to clear the session state 
explicitly, then we'd need to find a place after `OnExitProcess`.  I don't 
think such a hook exists right now, so perhaps we'd have to create one.  That 
hook could be the one place we reset `m_session_state` for both the successful 
and unsuccessful debug sessions.)

FYI:  I'll be offline until November 4.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D69503



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

[Lldb-commits] [PATCH] D69503: [LLDB] [Windows] Don't crash if the debugged process is unable to start up

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

In D69503#1726841 , @labath wrote:

> (Ideally, I'd just delete ProcessWindows entirely, and move everything to 
> lldb-server based model -- it's much easier to reason about threads there)


I believe there are people working on moving the Windows bits to the 
lldb-server model, but I don't think it's far enough along to get rid of 
ProcessWindows just yet.

The `m_mutex` is supposed to protect `m_session_data`.   `OnExitProcess` says:

  // No need to acquire the lock since m_session_data isn't accessed.

but it actually does access m_session_data.

So I think you've identified the cause of the race condition, but I'm not sure 
whether this is the best fix.  I need a little time to look at this more 
closely.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D69503



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

[Lldb-commits] [PATCH] D69612: [lldb-vscod] fix build with NDEBUG on windows

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

In D69612#1726849 , @SquallATF wrote:

> In D69612#1726829 , @labath wrote:
>
> > LGTM, modulo the `(void) result` stuff. Do you need someone to commit this 
> > for you?
>
>
> Yes I need.


Was the `(void) result` fix applied before this was committed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69612



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

[Lldb-commits] [PATCH] D69535: build: improve python check for Windows

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

In D69535#1724797 , @labath wrote:

> .. Given that this code is python3 specific and other platforms still support 
> python2, maybe we can make this bump windows-specific at first, and then 
> extend it to other platforms once we officially stop supporting python2 ?


Yes, please.  It's a mess on Windows, so I'll support a Windows-specific 
version bump.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D69535



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

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

FYI:  This broke the build for me.




Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:323
+  const char *function_name,
+  StructuredData::ObjectSP extra_args_sp) {}
 

This breaks some builds because it doesn't return a value.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68671



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

[Lldb-commits] [PATCH] D69366: [LLDB] [PECOFF] Fix symbols to refer to the right section

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:691
+  // so we should use section index "symbol.sect - 1 + 1".
+  Address symbol_addr(sect_list->GetSectionAtIndex(symbol.sect),
   symbol.value);

labath wrote:
> You should use FindSectionByID here. That's the API used by both ELF and 
> MachO object files and SymbolFilePDB classes for this sort of thing. This 
> would obviate the need for the long comment and make this patch independent 
> of the header stuff (which I'm going to commit as soon as I figure out how 
> that's done).
+1 to labath's suggestion.  It seems unfortunate that this already had a 
coupling to implementation details of the section list.


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

https://reviews.llvm.org/D69366



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

[Lldb-commits] [PATCH] D69100: COFF: Create a separate "section" for the file header

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth accepted this revision.
amccarth added a comment.

I'm OK with this.  I'm just wondering whether it would be a good idea to make 
it clear that these header sections are "not considered to be a section in the 
strictest sense."  Is the distinction going to be important to future code 
readers?  Do we already have "sections" that aren't truly "sections"?

Perhaps just a comment where the header sections are created?


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

https://reviews.llvm.org/D69100



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

[Lldb-commits] [PATCH] D68980: [LLDB] [test] Pass --target=-windows-msvc to clang-cl

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

>   I know there are some complexities with configuring msvc/clang-cl, where 
> one needs to fetch registry keys and whatnot in order to get the right build 
> environment.

My understanding is that the clang-cl driver does some poking around in the 
file system and registry to come up with defaults for the compatibility level, 
locations of the library and platform headers and such, but I believe that 
happens only if it needs to.  If all those answers are in the command line, it 
shouldn't bother inspecting the system.

I agree with Pavel in principle, but I'm not sure that finding a better general 
solution in the long run should hold this workaround up in the short term.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68980



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

[Lldb-commits] [PATCH] D68770: [LLDB] [Driver] Use llvm::InitLLVM to do unicode argument conversion on Windows

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

On Windows, it _does_ rewrite `argv[0]`, but it looks like it tries to not 
change whether it was relative/absolute, so I think this is fine.

Thanks for the clean-up!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68770



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

[Lldb-commits] [PATCH] D68770: [LLDB] [Driver] Use llvm::InitLLVM to do unicode argument conversion on Windows

[Adrian McCarthy via Phabricator via lldb-commits]

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

Cool.  I didn't know about InitLLVM.  That makes things much cleaner.

Though I do recall recently seeing another complaint about `argv[0]` not being 
preserved as typed but being replaced by an absolute path.  That will 
definitely happen now on Windows.  Is that a problem?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68770



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

[Lldb-commits] [PATCH] D68134: [LLDB] Use the llvm microsoft demangler instead of the windows dbghelp api

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth accepted this revision.
amccarth added a comment.

LGTM after one question.




Comment at: lldb/lit/SymbolFile/PDB/udt-layout.test:1
 REQUIRES: system-windows, lld
 RUN: %build --compiler=clang-cl --output=%t.exe %S/Inputs/UdtLayoutTest.cpp

Is `system-windows` still required after you've removed the dependency on 
dbghelp?


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

https://reviews.llvm.org/D68134



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

[Lldb-commits] [PATCH] D68258: [Windows] Introduce a switch for the `lldb-server` mode on Windows

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth accepted this revision.
amccarth added a comment.

Given Pavel's comment, this LGTM.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68258



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

[Lldb-commits] [PATCH] D61886: Support member functions construction and lookup in PdbAstBuilder

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.
Herald added a subscriber: JDevlieghere.

Is this still an active review or has this been abandoned?




Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:1047
+function_decl = m_clang.CreateFunctionDeclaration(
   parent, proc_name.str().c_str(), func_ct, storage, false);
+  }

It looks like `CreateFunctionDeclaration` can fail (returning a null pointer).  
Should this bail out if `function_decl` remains null pointer here (rather than 
crashing several lines later, after creating some bad mappings)?  Should we 
assert here to make debugging easier?

Also, nit, check the indentation here.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D61886



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

[Lldb-commits] [PATCH] D68258: [Windows] Introduce a switch for the `lldb-server` mode on Windows

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

In D68258#1690757 , @aleksandr.urakov 
wrote:

> In D68258#1690756 , 
> @aleksandr.urakov wrote:
>
> > I've made it in the way similar to Zachary have made for the 
> > `SymbolFileNativePDB` plugin. An environment variable could be more 
> > convenient e.g. to run a bunch of tests using the `lldb-test` option.
>


The environment variable for using the native PDB was always intended to be 
temporary, since the native PDB reader would eventually be the only PDB reader.

In general, I find environment variables harder to manage (especially on 
Windows) than command line options.  They're invisible global variables that 
compound all the environmental issues that make building and testing so flaky 
and hard to diagnose.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68258



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

[Lldb-commits] [PATCH] D68258: [Windows] Introduce a switch for the `lldb-server` mode on Windows

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

Why an environment variable rather than a command line option?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68258



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

[Lldb-commits] [PATCH] D67347: [Windows] Use information from the PE32 exceptions directory to construct unwind plans

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

I'm just back from vacation.  I agree with Pavel that we need to hear more from 
Jason at this point.  I'm still very interested in helping this land in some 
form.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67347



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

[Lldb-commits] [PATCH] D67347: [Windows] Use information from the PE32 exceptions directory to construct unwind plans

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

I have a couple more questions and some renaming requests.




Comment at: lldb/include/lldb/Symbol/DWARFCallFrameInfo.h:74
 
-  void ForEachFDEEntries(
-  const std::function 
);
+  void ForEachEntries(const std::function 
) override;
 

I find the name `ForEachEntries` confusing.  I know this is a leftover from the 
original code that you're modifying, but I wonder if it can get a better name.  
In particular, I don't know why it's plural, so I'd expect `ForEachEntry`, but 
even that is pretty vague.  I realize `FDE` is DWARF-specific, but at least it 
gives a clue as to what type of entries are involved.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.cpp:541
+const RuntimeFunction *PECallFrameInfo::FindRuntimeFunctionItersectsWithRange(
+const AddressRange ) const {
+  uint32_t rva = m_object_file.GetRVA(range.GetBaseAddress());

Isn't it possible for more than one RuntimeFunction to intersect with an 
address range?  In normal use, it might not happen because it's being called 
with constrained ranges, so maybe it's nothing to worry about.  I suppose if 
the range were too large and it encompassed several functions, returning any 
one of them is acceptable.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.h:46
+private:
+  const llvm::Win64EH::RuntimeFunction *FindRuntimeFunctionItersectsWithRange(
+  const lldb_private::AddressRange ) const;

nit:  missing `n`: FindRuntimeFunctionI**n**tersectsWithRange



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.h:50
+  ObjectFilePECOFF _object_file;
+  lldb_private::DataExtractor m_exception_data;
+};

`m_exception_data` is vague.  In the constructor, it's referred to as the 
exception directory, so perhaps `m_exception_dir` would be a bit more 
descriptive.



Comment at: lldb/source/Symbol/ObjectFile.cpp:687
+  return std::make_unique(*this, sect,
+  DWARFCallFrameInfo::EH);
+}

It seems a bit weird for DWARF-specific code to be here, when there are 
ObjectFile plugins for PECOFF, ELF, and Mach-O.  Obviously the 
PECOFFCallFrameInfo is instantiated in the PECOFF plugin.  The ELF and Mach-O 
versions instantiate DWARFCallFrameInfos.  Does the generic ObjectFile need to 
do the same?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67347



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

[Lldb-commits] [PATCH] D66638: Unwind: Add a stack scanning mechanism to support win32 unwinding

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth accepted this revision.
amccarth added a comment.

My concerns are satisfied.


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

https://reviews.llvm.org/D66638



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

[Lldb-commits] [PATCH] D67347: [Windows] Use information from the PE32 exceptions directory to construct unwind plans

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

I've been trying to figure out how to implement the same functionality you've 
got here, so I'm very interested in helping you land this in some form.

I think Clayborg and Pavel makes some good points about where the right layer 
of abstraction is for the unwind plans.  I've copied your patch locally and 
will try out some different ideas and report back.

This patch is pretty large.  It might be easier to break it up into a series of 
small steps to get there.  The bullet points in the CL description might be a 
good roadmap for such a break-up.  But let's first figure out the right 
abstraction points.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67347



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

[Lldb-commits] [PATCH] D67168: [Windows] Add support of watchpoints to `ProcessWindows`

[Adrian McCarthy via Phabricator via lldb-commits]

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

Thanks for the changes!  I think this looks good now.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67168



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

[Lldb-commits] [PATCH] D66638: Unwind: Add a stack scanning mechanism to support win32 unwinding

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

I don't have any specific code comments, but I do have a couple general 
questions and points to consider.

1. `isFoundHeuristically` is very generic.  It's true that it's a heuristic 
approach, but it's a very specific heuristic.  Might there be other heuristic 
approaches in the future?  Should we name it something more specific like 
`isRaSearch`?

2. `max_iterations` means how many stack positions the heuristic will scan 
before giving up, right?  Are there any alignment issues here?  Should we 
assert that the return address hint is a multiple of the stack alignment?

3. The 100 for `max_iterations` is probably fine, but I wonder if there's a way 
to determine a more specific limit without just guessing.  What things could be 
on the stack between the hint and the actual return address?  It seems like 
only arguments for a call that the current function is preparing to make.  The 
standard says that the actual number of parameters is implementation-defined, 
but that it suggests a minimum of 256.  Should `max_iterations` be 256?  Is 
there much risk in making it bigger than it needs to be?

4. Is checking for executable permission slow?  Would it be worth doing some 
culling or caching?  I imagine a lot of non-return address values on the stack 
will be simple small numbers, like 0 or 1, which, for Windows, would never be a 
valid executable address.




Comment at: include/lldb/Symbol/SymbolFile.h:254
+  /// variables and spilled registers, but it should not include paramenters, 
as
+  /// they are considered to be a part of the callers frame.
+  virtual llvm::Expected GetOwnFrameSize(Symbol ) {

Typos:

paramenters -> parameters
callers -> caller's



Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66638



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

[Lldb-commits] [PATCH] D66994: [lldb][CMake] Fix windows-x86-debug compilation with python enabled using multi-target generator

[Adrian McCarthy via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL371090: Fix windows-x86-debug compilation with python 
enabled using multi-target… (authored by amccarth, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66994?vs=218877=218946#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66994

Files:
  lldb/trunk/cmake/modules/LLDBConfig.cmake

Index: lldb/trunk/cmake/modules/LLDBConfig.cmake
===
--- lldb/trunk/cmake/modules/LLDBConfig.cmake
+++ lldb/trunk/cmake/modules/LLDBConfig.cmake
@@ -134,6 +134,48 @@
 #locate 64-bit Python libraries.
 # This function is designed to address those limitations.  Currently it only partially
 # addresses them, but it can be improved and extended on an as-needed basis.
+function(find_python_libs_windows_helper LOOKUP_DEBUG OUT_EXE_PATH_VARNAME OUT_LIB_PATH_VARNAME OUT_DLL_PATH_VARNAME OUT_VERSION_VARNAME)
+  if(LOOKUP_DEBUG)
+  set(POSTFIX "_d")
+  else()
+  set(POSTFIX "")
+  endif()
+
+  file(TO_CMAKE_PATH "${PYTHON_HOME}/python${POSTFIX}.exe"   PYTHON_EXE)
+  file(TO_CMAKE_PATH "${PYTHON_HOME}/libs/${PYTHONLIBS_BASE_NAME}${POSTFIX}.lib" PYTHON_LIB)
+  file(TO_CMAKE_PATH "${PYTHON_HOME}/${PYTHONLIBS_BASE_NAME}${POSTFIX}.dll"  PYTHON_DLL)
+
+  foreach(component PYTHON_EXE;PYTHON_LIB;PYTHON_DLL)
+if(NOT EXISTS ${${component}})
+  message(WARNING "Unable to find ${component}")
+  unset(${component})
+endif()
+  endforeach()
+
+  if (NOT PYTHON_EXE OR NOT PYTHON_LIB OR NOT PYTHON_DLL)
+message(WARNING "Unable to find all Python components.  Python support will be disabled for this build.")
+set(LLDB_DISABLE_PYTHON 1 PARENT_SCOPE)
+return()
+  endif()
+
+  # Find the version of the Python interpreter.
+  execute_process(COMMAND "${PYTHON_EXE}" -c
+  "import sys; sys.stdout.write('.'.join([str(x) for x in sys.version_info[:3]]))"
+  OUTPUT_VARIABLE PYTHON_VERSION_OUTPUT
+  RESULT_VARIABLE PYTHON_VERSION_RESULT
+  ERROR_QUIET)
+
+  if(PYTHON_VERSION_RESULT)
+message(WARNING "Unable to retrieve Python executable version")
+set(PYTHON_VERSION_OUTPUT "")
+  endif()
+
+  set(${OUT_EXE_PATH_VARNAME} ${PYTHON_EXE} PARENT_SCOPE)
+  set(${OUT_LIB_PATH_VARNAME} ${PYTHON_LIB} PARENT_SCOPE)
+  set(${OUT_DLL_PATH_VARNAME} ${PYTHON_DLL} PARENT_SCOPE)
+  set(${OUT_VERSION_VARNAME}  ${PYTHON_VERSION_OUTPUT} PARENT_SCOPE)
+endfunction()
+
 function(find_python_libs_windows)
   if ("${PYTHON_HOME}" STREQUAL "")
 message(WARNING "LLDB embedded Python on Windows requires specifying a value for PYTHON_HOME.  Python support disabled.")
@@ -161,55 +203,65 @@
   file(TO_CMAKE_PATH "${PYTHON_HOME}" PYTHON_HOME)
   # TODO(compnerd) when CMake Policy `CMP0091` is set to NEW, we should use
   # if(CMAKE_MSVC_RUNTIME_LIBRARY MATCHES MultiThreadedDebug)
-  if(CMAKE_BUILD_TYPE STREQUAL Debug)
-file(TO_CMAKE_PATH "${PYTHON_HOME}/python_d.exe" PYTHON_EXE)
-file(TO_CMAKE_PATH "${PYTHON_HOME}/libs/${PYTHONLIBS_BASE_NAME}_d.lib" PYTHON_LIB)
-file(TO_CMAKE_PATH "${PYTHON_HOME}/${PYTHONLIBS_BASE_NAME}_d.dll" PYTHON_DLL)
-  else()
-file(TO_CMAKE_PATH "${PYTHON_HOME}/python.exe" PYTHON_EXE)
-file(TO_CMAKE_PATH "${PYTHON_HOME}/libs/${PYTHONLIBS_BASE_NAME}.lib" PYTHON_LIB)
-file(TO_CMAKE_PATH "${PYTHON_HOME}/${PYTHONLIBS_BASE_NAME}.dll" PYTHON_DLL)
-  endif()
-
-  foreach(component PYTHON_EXE;PYTHON_LIB;PYTHON_DLL)
-if(NOT EXISTS ${${component}})
-  message(WARNING "unable to find ${component}")
-  unset(${component})
+  if(NOT DEFINED CMAKE_BUILD_TYPE)
+# Multi-target generator was selected (like Visual Studio or Xcode) where no concrete build type was passed
+# Lookup for both debug and release python installations
+find_python_libs_windows_helper(TRUE  PYTHON_DEBUG_EXE   PYTHON_DEBUG_LIB   PYTHON_DEBUG_DLL   PYTHON_DEBUG_VERSION_STRING)
+find_python_libs_windows_helper(FALSE PYTHON_RELEASE_EXE PYTHON_RELEASE_LIB PYTHON_RELEASE_DLL PYTHON_RELEASE_VERSION_STRING)
+if(LLDB_DISABLE_PYTHON)
+  set(LLDB_DISABLE_PYTHON 1 PARENT_SCOPE)
+  return()
 endif()
-  endforeach()
 
-  if (NOT PYTHON_EXE OR NOT PYTHON_LIB OR NOT PYTHON_DLL)
-message(WARNING "Unable to find all Python components.  Python support will be disabled for this build.")
-set(LLDB_DISABLE_PYTHON 1 PARENT_SCOPE)
-return()
+# We should have been found both debug and release python here
+# Now check that their versions are equal
+if(NOT PYTHON_DEBUG_VERSION_STRING STREQUAL PYTHON_RELEASE_VERSION_STRING)
+  message(FATAL_ERROR "Python versions for debug (${PYTHON_DEBUG_VERSION_STRING}) and release (${PYTHON_RELEASE_VERSION_STRING}) are different."
+  

[Lldb-commits] [PATCH] D66994: [lldb][CMake] Fix windows-x86-debug compilation with python enabled using multi-target generator

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

Yes, I can commit it for you soon.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66994



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

[Lldb-commits] [PATCH] D66994: [lldb][CMake] Fix windows-x86-debug compilation with python enabled using multi-target generator

[Adrian McCarthy via Phabricator via lldb-commits]

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

Thanks for factoring out the duplication.

Fingers crossed.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66994



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

[Lldb-commits] [PATCH] D67168: [Windows] Add support of watchpoints to `ProcessWindows`

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added inline comments.



Comment at: lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp:397
+if (slot != LLDB_INVALID_INDEX32) {
+  int id = m_watchpoint_slots[slot];
+  LLDB_LOG(log,

The names here are a bit confusing:  `GetTriggeredHardwareBreakpointSlot` 
doesn't return a slot; it returns the index of a slot, so  `slot` also isn't a 
slot, but the index of a slot.  `m_watchpoint_slots` is not a collection of 
slots but IDs, that's indexed by an index called a `slot`.

It may not be possible to completely rationalize this, but doing even a little 
could help future readers understand.  For example, `slot` could be 
`slot_index`, and `m_watchpoint_slots` could be `m_watchpoint_ids` (or even 
just `m_watchpoints`).



Comment at: lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp:525
   for (const auto _info : m_session_data->m_new_threads) {
-ThreadSP thread(new TargetThreadWindows(*this, thread_info.second));
-thread->SetID(thread_info.first);
-new_thread_list.AddThread(thread);
+new_thread_list.AddThread(thread_info.second);
 ++new_size;

This no longer sets the thread ID.  Was that intentional?



Comment at: lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp:743
 
-void ProcessWindows::OnCreateThread(const HostThread _thread) {
+void ProcessWindows::OnCreateThread(HostThread _thread) {
   llvm::sys::ScopedLock lock(m_mutex);

Can you help me why we needed to get rid of the `const` on the `HostThread &`?

If `native_new_thread` were also a const ref, I don't see any conflicting 
constraint.



Comment at: lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp:832
+  Status error;
+
+  WatchpointInfo info;

Should we check if the watchpoint is already enabled?  I noticed that 
`DisableWatchpoint` has an analogous guard clause.



Comment at: lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp:852
+RegisterContextWindows *reg_ctx = static_cast(
+thread->GetRegisterContext().get());
+if (!reg_ctx->AddHardwareBreakpoint(info.slot, info.address, info.size,

Since you have to explicitly downcast, wouldn't `auto *reg_ctx` on the left be 
sufficient and just as clear?



Comment at: 
lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp:82
 
-bool RegisterContextWindows::ClearHardwareBreakpoint(uint32_t hw_idx) {
-  return false;
-}
+  if (!size || size > 8 || size & (size - 1))
+return false;

Clever!  It took me a minute or two to figure out what the point of that was 
checking.  Perhaps a comment to explain?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67168



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

[Lldb-commits] [PATCH] D67123: [lldb] Early exit in RangeDataVector:FindEntryIndexesThatContain

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

Disregard my last comment.  Phabricator wasn't showing me that latest, nor that 
the patch had already been submitted, which seems to be happening with 
increasing frequency.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67123



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

[Lldb-commits] [PATCH] D67123: [lldb] Early exit in RangeDataVector:FindEntryIndexesThatContain

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added inline comments.



Comment at: lldb/include/lldb/Utility/RangeMap.h:739
+auto end = std::lower_bound(m_entries.begin(), m_entries.end(),
+Entry(addr, 1), BaseLessEqual);
+for (auto I = m_entries.begin(); I != end; ++I) {

teemperor wrote:
> labath wrote:
> > amccarth wrote:
> > > You're trying to find an upper bound, so `std::upper_bound` seems more 
> > > natural than `std::lower_bound`.  Understanding how `std::lower_bound` 
> > > works with a comparator that implements `<=` requires some mental 
> > > gymnastics.  With `std::upper_bound`, you'd just need `<` that compares 
> > > only the base addresses.
> > > 
> > > You could even avoid the custom comparison function by using the maximum 
> > > value for the size:
> > > 
> > > ```
> > > auto end = std::upper_bound(m_entries.begin(), m_entries.end(),
> > > Entry(addr, 
> > > std::numeric_limits::max()));
> > > ```
> > > 
> > Actually, If I understand this correctly, there is no need for either lower 
> > or upper bound here. Since you're going to be iterating through the list 
> > anyway. It should be sufficient to add a early exit from the loop once you 
> > encounter an element whose base address is >= the address you search for.
> Oh true, I got confused by all the other lower_bounding we do in the 
> surrounding functions :)
I still don't see the early exit from the loop.  Have you uploaded the latest 
diff to Phabricator?

(Thanks Pavel for pointing out the obvious that teemperor and I both missed.)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67123



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

[Lldb-commits] [PATCH] D67067: Breakpad: Basic support for STACK WIN unwinding

[Adrian McCarthy via Phabricator via lldb-commits]

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

LGTM.  Please consider adding a comment to the assert that summarizes what you 
explained in the thread.




Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:549
+  llvm::Optional record = StackWinRecord::parse(*It);
+  assert(record.hasValue());
+

labath wrote:
> amccarth wrote:
> > Should we log and bail out rather than just assert?  A corrupt symbol file 
> > shouldn't kill the debugger, right?
> > 
> > Also, it's Optional rather than Expected, so it seems even more plausible 
> > to hit this.
> This is an internal consistency check. An entry will be added to the 
> `m_unwind_data->win` map only if it was already parsed successfully down in 
> `ParseUnwindData`. This is parsing the same data once more, so it should 
> always succeed.
> 
> Now the next question is, why parse the same data twice? :)
> The first parse is to build an index of the ranges covered by the breakpad 
> file. In the second pass we actually parse the undwind data. Theoretically we 
> could avoid the second parse if we stored more data in the first one. 
> However, here I am operating under the assumption that most record will not 
> be touched, so it's worth to save some space for *each* record for the price 
> of having to parse twice *some* of them. This seems like a good tradeoff 
> intuitively, but I am don't have hard data to back that up.
> 
> Also, the case was much stronger for STACK CFI records (which do the same 
> thing), as there I only have to put the STACK CFI INIT records into the map 
> (and each INIT record is followed by a potentially large number of non-INIT 
> records). STACK WIN records don't have an INIT records, so I have to insert 
> all of them anyway, which makes the savings smaller, but it still seems worth 
> it.
Cool.  Could you just add a comment at the assertion that says a short version 
of that.  For example, "We've already parsed it once, so it shouldn't fail this 
time."


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

https://reviews.llvm.org/D67067



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

[Lldb-commits] [PATCH] D67067: Breakpad: Basic support for STACK WIN unwinding

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

This is looking pretty good.

I'm wondering whether it needs some "negative" tests.




Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:549
+  llvm::Optional record = StackWinRecord::parse(*It);
+  assert(record.hasValue());
+

Should we log and bail out rather than just assert?  A corrupt symbol file 
shouldn't kill the debugger, right?

Also, it's Optional rather than Expected, so it seems even more plausible to 
hit this.



Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:586
+  // We assume the first value will be the CFA. It is usually called T0, but
+  // clang will use T1, if it needs to realing the stack.
+  if (!postfix::ResolveSymbols(it->second, symbol_resolver)) {

Typo: realign


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

https://reviews.llvm.org/D67067



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

[Lldb-commits] [PATCH] D67083: [dotest] Avoid the need for LEVEL= makefile boilerplate

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth accepted this revision.
amccarth added a comment.

Nice!


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

https://reviews.llvm.org/D67083



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

[Lldb-commits] [PATCH] D67123: [lldb] Use binary search in RangeDataVector:FindEntryIndexesThatContain

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

This feels very familiar.  I think I've reviewed a similar change back when we 
first implemented minidumps.




Comment at: lldb/include/lldb/Utility/RangeMap.h:739
+auto end = std::lower_bound(m_entries.begin(), m_entries.end(),
+Entry(addr, 1), BaseLessEqual);
+for (auto I = m_entries.begin(); I != end; ++I) {

You're trying to find an upper bound, so `std::upper_bound` seems more natural 
than `std::lower_bound`.  Understanding how `std::lower_bound` works with a 
comparator that implements `<=` requires some mental gymnastics.  With 
`std::upper_bound`, you'd just need `<` that compares only the base addresses.

You could even avoid the custom comparison function by using the maximum value 
for the size:

```
auto end = std::upper_bound(m_entries.begin(), m_entries.end(),
Entry(addr, 
std::numeric_limits::max()));
```



Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67123



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

[Lldb-commits] [PATCH] D66994: [lldb][CMake] Fix windows-x86-debug compilation with python enabled using multi-target generator

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

I'm open to this if we can reduce the code duplication.

One of my concerns is that changes in July and August completely broke the 
build for me for many days.  I had to remove all but one version of Python to 
ensure that it always found the right one.

I, too, would appreciate being able to run tests under the debugger, but I find 
that's impossible with python_d because it detects and halts virtually every 
test (like using the Python allocator without holding the GIL and leaked 
resource warnings).


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66994



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

[Lldb-commits] [PATCH] D66994: [lldb][CMake] Fix windows-x86-debug compilation with python enabled using multi-target generator

[Adrian McCarthy via Phabricator via lldb-commits]

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



Comment at: cmake/modules/LLDBConfig.cmake:164
   # if(CMAKE_MSVC_RUNTIME_LIBRARY MATCHES MultiThreadedDebug)
-  if(CMAKE_BUILD_TYPE STREQUAL Debug)
-file(TO_CMAKE_PATH "${PYTHON_HOME}/python_d.exe" PYTHON_EXE)
-file(TO_CMAKE_PATH "${PYTHON_HOME}/libs/${PYTHONLIBS_BASE_NAME}_d.lib" 
PYTHON_LIB)
-file(TO_CMAKE_PATH "${PYTHON_HOME}/${PYTHONLIBS_BASE_NAME}_d.dll" 
PYTHON_DLL)
-  else()
-file(TO_CMAKE_PATH "${PYTHON_HOME}/python.exe" PYTHON_EXE)
-file(TO_CMAKE_PATH "${PYTHON_HOME}/libs/${PYTHONLIBS_BASE_NAME}.lib" 
PYTHON_LIB)
-file(TO_CMAKE_PATH "${PYTHON_HOME}/${PYTHONLIBS_BASE_NAME}.dll" PYTHON_DLL)
-  endif()
-
-  foreach(component PYTHON_EXE;PYTHON_LIB;PYTHON_DLL)
-if(NOT EXISTS ${${component}})
-  message(WARNING "unable to find ${component}")
-  unset(${component})
+  if (NOT DEFINED CMAKE_BUILD_TYPE OR CMAKE_BUILD_TYPE STREQUAL Debug)
+file(TO_CMAKE_PATH "${PYTHON_HOME}/python_d.exe"   
PYTHON_DEBUG_EXE)

This first changed line seems useful.

The rest seems of dubious benefit, even if the duplication can be reduced.  Are 
folks really using the VS project to build both Debug and Release?  I'd be 
surprised, since the debug version won't pass half of the tests.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66994



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

[Lldb-commits] [PATCH] D66994: [lldb][CMake] Fix windows-x86-debug compilation with python enabled using multi-target generator

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

I'll look at this in detail soon, but a few caveats:

- lldb tests don't work with the debug Python interpreter, which is why I've 
been complaining about test failures approximately forever when the bots seem 
fine.

- Some of these changes you're undoing caused a lot of pain when they were 
first put in, so let's be really sure if we need to undo this.

- I use the VS project only for code browsing and debugging.  For building and 
tests, I use the ninja project.  So I'm not very familiar with the problem 
you're trying to fix.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66994



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

[Lldb-commits] [PATCH] D66841: Skip test_target_create_invalid_core_file on Windows

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth abandoned this revision.
amccarth added a comment.

Somebody else already did this while I was waiting for review.


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

https://reviews.llvm.org/D66841



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

[Lldb-commits] [PATCH] D66863: [lldb] Unify target checking in CommandObject

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

I support anything that reduces the code path differences between user-entered 
commands and their SBAPI counterparts.  Thanks for doing this!


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66863



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

[Lldb-commits] [PATCH] D66634: Postfix: move more code out of the PDB plugin

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

When I added my comments, Phabricator showed this patch as still open.  Now it 
looks like it landed four hours before that. :-(


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66634



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

[Lldb-commits] [PATCH] D66841: Skip test_target_create_invalid_core_file on Windows

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth created this revision.
amccarth added a reviewer: teemperor.

The operation is supposed to fail, but apparently it fails on Windows for a 
different reason than the test anticipated.


https://reviews.llvm.org/D66841

Files:
  
lldb/packages/Python/lldbsuite/test/functionalities/target_command/TestTargetCommand.py


Index: 
lldb/packages/Python/lldbsuite/test/functionalities/target_command/TestTargetCommand.py
===
--- 
lldb/packages/Python/lldbsuite/test/functionalities/target_command/TestTargetCommand.py
+++ 
lldb/packages/Python/lldbsuite/test/functionalities/target_command/TestTargetCommand.py
@@ -341,6 +341,7 @@
 self.expect("target create -s doesntexist doesntexisteither", 
error=True,
 substrs=["invalid symbol file path 'doesntexist'"])
 
+@skipIfWindows
 @no_debug_info_test
 def test_target_create_invalid_core_file(self):
 invalid_core_path = os.path.join(self.getSourceDir(), 
"invalid_core_file")


Index: lldb/packages/Python/lldbsuite/test/functionalities/target_command/TestTargetCommand.py
===
--- lldb/packages/Python/lldbsuite/test/functionalities/target_command/TestTargetCommand.py
+++ lldb/packages/Python/lldbsuite/test/functionalities/target_command/TestTargetCommand.py
@@ -341,6 +341,7 @@
 self.expect("target create -s doesntexist doesntexisteither", error=True,
 substrs=["invalid symbol file path 'doesntexist'"])
 
+@skipIfWindows
 @no_debug_info_test
 def test_target_create_invalid_core_file(self):
 invalid_core_path = os.path.join(self.getSourceDir(), "invalid_core_file")
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D66634: Postfix: move more code out of the PDB plugin

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

I have a couple inline questions.  After that, it looks fine.




Comment at: 
lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:61
 
+  for (auto it = parsed.begin(), end = parsed.end(); it != end; ++it) {
 // Emplace valid dependent subtrees to make target assignment independent

I would recommend making `parsed` a `const` vector.  The lambda captures the 
iterator, and while the code currently looks fine, I'd hate for something to 
change in the future that could invalidate the captured iterator.



Comment at: 
lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:69
+  return pair.second;
+  }
+

This looks O(n^2).  Will that matter here or will the expressions always be 
short?



Comment at: 
lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:84
   // found target assignment program - no need to parse further
-  return rvalue_ast;
+  return it->second;
 }

It's a shame the `pair` loses the more descriptive field names, but I don't see 
a good way to make it better, so this is a useless comment.



Comment at: lldb/trunk/source/Symbol/PostfixExpression.cpp:101
+if (!rhs)
+  return {};
+result.emplace_back(lhs, rhs);

Is it correct to return an empty vector here rather than `result`?  If there's 
one bad expression, you'll consider the entire FPO program empty.  That's 
sounds plausible, but I thought I'd ask.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66634



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

[Lldb-commits] [PATCH] D66633: Breakpad: Add support for parsing STACK WIN records

[Adrian McCarthy via Phabricator via lldb-commits]

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

LGTM.


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

https://reviews.llvm.org/D66633



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

[Lldb-commits] [PATCH] D66447: Add char8_t support (C++20)

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

A couple inline comments.  I think this is looking pretty good.




Comment at: 
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/char8_t/main.cpp:1
+#include 
+

Is this include necessary?



Comment at: lldb/trunk/source/Symbol/ClangASTContext.cpp:1391
   return CompilerType(this, ast->Char32Ty.getAsOpaquePtr());
-}
+else if (streq(type_name, "char8_t"))
+  return CompilerType(this, ast->Char8Ty.getAsOpaquePtr());

I think the current style is to omit the `else` keywords on these, since each 
`if` returns.  That would at least be consistent with several cases above.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66447



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

[Lldb-commits] [PATCH] D66445: Explicitly Cast Constants to DWORD

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

Is this necessary?  I see

  C++
  #define STATUS_BREAKPOINT((DWORD   )0x8003L)
  #define STATUS_SINGLE_STEP   ((DWORD   )0x8004L)

in  C:\Program Files (x86)\Windows Kits\8.1\Include\um\winnt.h


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66445



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