[Lldb-commits] [PATCH] D55614: Fix MinidumpParser::GetFilteredModuleList() and test it

[Pavel Labath via Phabricator via lldb-commits]

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

Looks good to me.


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

https://reviews.llvm.org/D55614



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

[Lldb-commits] [PATCH] D55582: [Reproducers] Add command reproducer

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I am very sorry about this, but I am having some second thoughts about the 
`RunReplay` thingy. I don't think I've correctly expressed what is bothering me 
about it (and that's probably because I wasn't clear with myself about what is 
bothering me). I'll try to formulate it better this time

What I was thinking about were the boundaries at which we want to record/replay 
events. If I understand correctly, one of those boundaries should be the SB 
layer (is that correct?). So, with that in mind, it makes sense for the lldb 
driver to do something "special" in replay mode, and invoke some other function 
which would replay the SB calls it would have normally made. That function 
might as well be called `RunReplay`.

Now, if we imagine we have this SB record/replay capability, during recording 
it would detect that the driver made a call to 
`SBDebugger::RunCommandInterpreter`. Then, during recording it would again make 
a call to SBDebugger::RunCommandInterpreter, but ensure that it behaves the 
same way as it did the first time (this is the auto-detection part I was 
speaking about).

The reason this came out all wrong is because I was also thinking about a 
(somewhat) unrelated thing, and that is the layer at which we do the capturing 
of the commands. I was thinking that maybe the command interpreter doesn't need 
to know anything about the record/replay taking place. If this SB replay engine 
substituted the `stdin` of the debugger with a recorded version of it, then it 
could go normally go about it's business and things would "just work" (you'd 
still have to make sure that libedit doesn't do something funny, but that could 
hopefully be done at the IOHandler level by substituting the editline IO 
handler).

So, I mean, that's how I'd do it, but this is your feature, and where you wan't 
to draw the record/replay line ultimately depends on what you want to get out 
of the feature. However, I do see some advantages to doing the r/r at a lower 
level that I'd like to try to sell to you:

- better possibility for capturing ^C. I think ^C is fairly important, 
particularly in console debugging, as it's the only way to interrupt a program 
without it voluntarily hitting a breakpoint. It will also be fairly tricky to 
reproduce. With the current framework, I'm not sure how you would manage to 
capture that. If you worked with IO handlers and was capturing stdin, I think 
you'd have a better chance of getting it right (since both stdin and ^C come 
from the terminal).
- no need for special handling of command sourcing. As you would capture 
almost-raw stdin, you wouldn't need to do anything special to the avoid sourced 
commands being captured twice. The filesystem would just provide the captured 
file, and stdin-recorder would capture the stdin. (which I think would make the 
stdin-capturer fit nicely with the Filesystem one, as they're both fairly 
low-level).
- right now you're merging commands coming from multiple sources (stdin via 
`RunCommandInterpreter`, command strings from `HandleCommand`) into one 
monolithic command stream. I think this means that once you have an SB 
recorder, it will also have to special case command APIs to avoid doing things 
twice. If you work at a lower level, this won't happen, because HandleCommand 
would be naturally captured as part of the SB stream, and stdin commands would 
be captured by the stdin-recorder (which would probably also be initialized and 
managed by the SB recorder, since it needs to be hooked into 
RunCommandInterpreter)

So, basically, my point is:

- let's bring back `RunReplay`. I don't think it makes sense to force this to 
go through `SBDebugger::RunCommandInterpreter` in the current setup, since both 
the caller and the callee special-case replay mode anyway.
- I'd urge you to consider whether you really want to do the capturing at this 
level


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

https://reviews.llvm.org/D55582



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

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: source/Commands/CommandObjectTarget.cpp:4246
   if (symbol_file) {
-ObjectFile *object_file = symbol_file->GetObjectFile();
 

lemo wrote:
> note I had to bypass this check: we don't (yet) have a ObjectFilePDB so the 
> SymbolFileNativePDB always points to the associated PE binary. 
> 
> the check itself seems valuable as a diagnostic but not strictly required. 
> Should I add a TODO comment and/or open a bug to revisit this?
I not sure this is a good idea. Isn't this the only way of providing feedback 
about whether the symbols were actually added? If we are unable to load the 
symbol file specified (perhaps because the user made a typo, or the file is 
corrupted), then the symbol vendor will just create a default SymbolFile backed 
by the original object file. Doesn't that mean this will basically always 
return true now?

I think this is strictly worse that the previous solution as it lets the 
objectless-symbol-file hack leak out of SymbolFilePDB.


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

https://reviews.llvm.org/D55142



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

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via lldb-commits]

On irc earlier i was talking about this with Greg and he said it should be
fine in his opinion. I’ll point him to this review in the morning so he can
comment
On Thu, Dec 13, 2018 at 3:30 AM Pavel Labath via Phabricator <
revi...@reviews.llvm.org> wrote:

> labath added inline comments.
>
>
> 
> Comment at: source/Commands/CommandObjectTarget.cpp:4246
>if (symbol_file) {
> -ObjectFile *object_file = symbol_file->GetObjectFile();
>
> 
> lemo wrote:
> > note I had to bypass this check: we don't (yet) have a ObjectFilePDB so
> the SymbolFileNativePDB always points to the associated PE binary.
> >
> > the check itself seems valuable as a diagnostic but not strictly
> required. Should I add a TODO comment and/or open a bug to revisit this?
> I not sure this is a good idea. Isn't this the only way of providing
> feedback about whether the symbols were actually added? If we are unable to
> load the symbol file specified (perhaps because the user made a typo, or
> the file is corrupted), then the symbol vendor will just create a default
> SymbolFile backed by the original object file. Doesn't that mean this will
> basically always return true now?
>
> I think this is strictly worse that the previous solution as it lets the
> objectless-symbol-file hack leak out of SymbolFilePDB.
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D55142/new/
>
> https://reviews.llvm.org/D55142
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] r349027 - Classify tests in lit/Modules

[Pavel Labath via lldb-commits]

Author: labath
Date: Thu Dec 13 04:13:29 2018
New Revision: 349027

URL: http://llvm.org/viewvc/llvm-project?rev=349027&view=rev
Log:
Classify tests in lit/Modules

We've recently developed a convention where the tests are placed into
subfolders according to the object file type. This applies that
convention to existing tests too.

Added:
lldb/trunk/lit/Modules/ELF/
lldb/trunk/lit/Modules/ELF/build-id-case.yaml
  - copied, changed from r348936, lldb/trunk/lit/Modules/build-id-case.yaml
lldb/trunk/lit/Modules/ELF/compressed-sections.yaml
  - copied, changed from r348936, 
lldb/trunk/lit/Modules/compressed-sections.yaml
lldb/trunk/lit/Modules/ELF/duplicate-section.yaml
  - copied, changed from r348936, 
lldb/trunk/lit/Modules/elf-duplicate-section.yaml
lldb/trunk/lit/Modules/ELF/many-sections.s
  - copied, changed from r348936, lldb/trunk/lit/Modules/elf-many-sections.s
lldb/trunk/lit/Modules/ELF/section-types.yaml
  - copied, changed from r348936, 
lldb/trunk/lit/Modules/elf-section-types.yaml
lldb/trunk/lit/Modules/ELF/short-build-id.yaml
  - copied, changed from r348936, lldb/trunk/lit/Modules/short-build-id.yaml
lldb/trunk/lit/Modules/MachO/lc_build_version.yaml
  - copied, changed from r348936, 
lldb/trunk/lit/Modules/lc_build_version.yaml
lldb/trunk/lit/Modules/MachO/lc_build_version_notools.yaml
  - copied, changed from r348936, 
lldb/trunk/lit/Modules/lc_build_version_notools.yaml
lldb/trunk/lit/Modules/MachO/lc_version_min.yaml
  - copied, changed from r348936, lldb/trunk/lit/Modules/lc_version_min.yaml
Removed:
lldb/trunk/lit/Modules/build-id-case.yaml
lldb/trunk/lit/Modules/compressed-sections.yaml
lldb/trunk/lit/Modules/elf-duplicate-section.yaml
lldb/trunk/lit/Modules/elf-many-sections.s
lldb/trunk/lit/Modules/elf-section-types.yaml
lldb/trunk/lit/Modules/lc_build_version.yaml
lldb/trunk/lit/Modules/lc_build_version_notools.yaml
lldb/trunk/lit/Modules/lc_version_min.yaml
lldb/trunk/lit/Modules/short-build-id.yaml

Copied: lldb/trunk/lit/Modules/ELF/build-id-case.yaml (from r348936, 
lldb/trunk/lit/Modules/build-id-case.yaml)
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Modules/ELF/build-id-case.yaml?p2=lldb/trunk/lit/Modules/ELF/build-id-case.yaml&p1=lldb/trunk/lit/Modules/build-id-case.yaml&r1=348936&r2=349027&rev=349027&view=diff
==
(empty)

Copied: lldb/trunk/lit/Modules/ELF/compressed-sections.yaml (from r348936, 
lldb/trunk/lit/Modules/compressed-sections.yaml)
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Modules/ELF/compressed-sections.yaml?p2=lldb/trunk/lit/Modules/ELF/compressed-sections.yaml&p1=lldb/trunk/lit/Modules/compressed-sections.yaml&r1=348936&r2=349027&rev=349027&view=diff
==
(empty)

Copied: lldb/trunk/lit/Modules/ELF/duplicate-section.yaml (from r348936, 
lldb/trunk/lit/Modules/elf-duplicate-section.yaml)
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Modules/ELF/duplicate-section.yaml?p2=lldb/trunk/lit/Modules/ELF/duplicate-section.yaml&p1=lldb/trunk/lit/Modules/elf-duplicate-section.yaml&r1=348936&r2=349027&rev=349027&view=diff
==
(empty)

Copied: lldb/trunk/lit/Modules/ELF/many-sections.s (from r348936, 
lldb/trunk/lit/Modules/elf-many-sections.s)
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Modules/ELF/many-sections.s?p2=lldb/trunk/lit/Modules/ELF/many-sections.s&p1=lldb/trunk/lit/Modules/elf-many-sections.s&r1=348936&r2=349027&rev=349027&view=diff
==
(empty)

Copied: lldb/trunk/lit/Modules/ELF/section-types.yaml (from r348936, 
lldb/trunk/lit/Modules/elf-section-types.yaml)
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Modules/ELF/section-types.yaml?p2=lldb/trunk/lit/Modules/ELF/section-types.yaml&p1=lldb/trunk/lit/Modules/elf-section-types.yaml&r1=348936&r2=349027&rev=349027&view=diff
==
(empty)

Copied: lldb/trunk/lit/Modules/ELF/short-build-id.yaml (from r348936, 
lldb/trunk/lit/Modules/short-build-id.yaml)
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Modules/ELF/short-build-id.yaml?p2=lldb/trunk/lit/Modules/ELF/short-build-id.yaml&p1=lldb/trunk/lit/Modules/short-build-id.yaml&r1=348936&r2=349027&rev=349027&view=diff
==
(empty)

Copied: lldb/trunk/lit/Modules/MachO/lc_build_version.yaml (from r348936, 
lldb/trunk/lit/Modules/lc_build_version.yaml)
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Modules/MachO/lc_build_version.yaml?p2=lldb/trunk/lit/Modules/MachO/lc_build_version.yaml&p1=lldb/trunk/lit/Modules/lc

[Lldb-commits] [PATCH] D55653: Check pointer results on nullptr before using them

[Tatyana Krasnukha via Phabricator via lldb-commits]

tatyana-krasnukha created this revision.
tatyana-krasnukha added reviewers: ki.stfu, abidh.
Herald added a subscriber: lldb-commits.

Prevent crashing like llvm.org/pr37054


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D55653

Files:
  MICmdCmdData.cpp
  MICmdCmdGdbInfo.cpp
  MICmdCmdMiscellanous.cpp
  MICmnLLDBDebugSessionInfo.cpp
  MIDataTypes.h

Index: MIDataTypes.h
===
--- MIDataTypes.h
+++ MIDataTypes.h
@@ -62,3 +62,18 @@
 // Fundamentals:
 typedef long long MIint64;   // 64bit signed integer.
 typedef unsigned long long MIuint64; // 64bit unsigned integer.
+
+template 
+class MIDereferencable {
+public:
+  MIDereferencable(T *ptr) : m_ptr(ptr) {}
+
+  T *Or(T &alt_value) { return m_ptr ? m_ptr : &alt_value; }
+private:
+  T* m_ptr = nullptr;
+};
+
+template
+MIDereferencable GetDereferencable(T *ptr) {
+  return MIDereferencable(ptr);
+}
\ No newline at end of file
Index: MICmnLLDBDebugSessionInfo.cpp
===
--- MICmnLLDBDebugSessionInfo.cpp
+++ MICmnLLDBDebugSessionInfo.cpp
@@ -639,11 +639,10 @@
 
   vwPc = rFrame.GetPC();
 
-  const char *pFnName = rFrame.GetFunctionName();
-  vwFnName = (pFnName != nullptr) ? pFnName : pUnkwn;
+  vwFnName = GetDereferencable(rFrame.GetFunctionName()).Or(*pUnkwn);
 
-  const char *pFileName = rFrame.GetLineEntry().GetFileSpec().GetFilename();
-  vwFileName = (pFileName != nullptr) ? pFileName : pUnkwn;
+  vwFileName = GetDereferencable(
+  rFrame.GetLineEntry().GetFileSpec().GetFilename()).Or(*pUnkwn);
 
   vwnLine = rFrame.GetLineEntry().GetLine();
 
@@ -829,9 +828,9 @@
   vrwBrkPtInfo.m_id = vBrkPt.GetID();
   vrwBrkPtInfo.m_strType = "breakpoint";
   vrwBrkPtInfo.m_pc = nAddr;
-  vrwBrkPtInfo.m_fnName = pFn;
-  vrwBrkPtInfo.m_fileName = pFile;
-  vrwBrkPtInfo.m_path = pFilePath;
+  vrwBrkPtInfo.m_fnName = GetDereferencable(pFn).Or(*pUnkwn);
+  vrwBrkPtInfo.m_fileName = GetDereferencable(pFile).Or(*pUnkwn);
+  vrwBrkPtInfo.m_path = GetDereferencable(pFilePath).Or(*pUnkwn);
   vrwBrkPtInfo.m_nLine = nLine;
   vrwBrkPtInfo.m_nTimes = vBrkPt.GetHitCount();
 
Index: MICmdCmdMiscellanous.cpp
===
--- MICmdCmdMiscellanous.cpp
+++ MICmdCmdMiscellanous.cpp
@@ -336,9 +336,12 @@
 }
 
 if (rSessionInfo.GetTarget().IsValid()) {
+  const char *pUnkwn = "??";
   lldb::SBTarget sbTrgt = rSessionInfo.GetTarget();
-  const char *pDir = sbTrgt.GetExecutable().GetDirectory();
-  const char *pFileName = sbTrgt.GetExecutable().GetFilename();
+  const char *pDir =
+  GetDereferencable(sbTrgt.GetExecutable().GetDirectory()).Or(*pUnkwn);
+  const char *pFileName =
+  GetDereferencable(sbTrgt.GetExecutable().GetFilename()).Or(*pUnkwn);
   const CMIUtilString strFile(
   CMIUtilString::Format("%s/%s", pDir, pFileName));
   const CMICmnMIValueConst miValueConst4(strFile);
Index: MICmdCmdGdbInfo.cpp
===
--- MICmdCmdGdbInfo.cpp
+++ MICmdCmdGdbInfo.cpp
@@ -192,6 +192,8 @@
   bool bOk = CMICmnStreamStdout::TextToStdout(
   "~\"FromTo  Syms Read   Shared Object Library\"");
 
+  const char *pUnknown = "??";
+
   CMICmnLLDBDebugSessionInfo &rSessionInfo(
   CMICmnLLDBDebugSessionInfo::Instance());
   lldb::SBTarget sbTarget = rSessionInfo.GetTarget();
@@ -199,9 +201,11 @@
   for (MIuint i = 0; bOk && (i < nModules); i++) {
 lldb::SBModule module = sbTarget.GetModuleAtIndex(i);
 if (module.IsValid()) {
+  auto fileSpec = module.GetFileSpec();
   const CMIUtilString strModuleFilePath(
-  module.GetFileSpec().GetDirectory());
-  const CMIUtilString strModuleFileName(module.GetFileSpec().GetFilename());
+  GetDereferencable(fileSpec.GetDirectory()).Or(*pUnknown));
+  const CMIUtilString strModuleFileName(
+  GetDereferencable(fileSpec.GetFilename()).Or(*pUnknown));
   const CMIUtilString strModuleFullPath(CMIUtilString::Format(
   "%s/%s", strModuleFilePath.c_str(), strModuleFileName.c_str()));
   const CMIUtilString strHasSymbols =
Index: MICmdCmdData.cpp
===
--- MICmdCmdData.cpp
+++ MICmdCmdData.cpp
@@ -373,8 +373,8 @@
   for (size_t i = 0; i < nInstructions; i++) {
 const char *pUnknown = "??";
 lldb::SBInstruction instrt = instructions.GetInstructionAtIndex(i);
-const char *pStrMnemonic = instrt.GetMnemonic(sbTarget);
-pStrMnemonic = (pStrMnemonic != nullptr) ? pStrMnemonic : pUnknown;
+const char *pStrMnemonic =
+GetDereferencable(instrt.GetMnemonic(sbTarget)).Or(*pUnknown);
 const char *pStrComment = instrt.GetComment(sbTarget);
 CMIUtilString strComment;
 if (pStrComment != nullptr && *pStrComment != '\0')
@@ -381,11 +381,11 @@
   strComment = CMIUtilString::Format("; %s"

[Lldb-commits] [lldb] r349036 - Add missing Initialize/Terminate for Architecture plugins

[Tatyana Krasnukha via lldb-commits]

Author: tkrasnukha
Date: Thu Dec 13 06:28:25 2018
New Revision: 349036

URL: http://llvm.org/viewvc/llvm-project?rev=349036&view=rev
Log:
Add missing Initialize/Terminate for Architecture plugins

Modified:
lldb/trunk/source/API/SystemInitializerFull.cpp

Modified: lldb/trunk/source/API/SystemInitializerFull.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/API/SystemInitializerFull.cpp?rev=349036&r1=349035&r2=349036&view=diff
==
--- lldb/trunk/source/API/SystemInitializerFull.cpp (original)
+++ lldb/trunk/source/API/SystemInitializerFull.cpp Thu Dec 13 06:28:25 2018
@@ -40,6 +40,7 @@
 #include "Plugins/ABI/SysV-s390x/ABISysV_s390x.h"
 #include "Plugins/ABI/SysV-x86_64/ABISysV_x86_64.h"
 #include "Plugins/Architecture/Arm/ArchitectureArm.h"
+#include "Plugins/Architecture/Mips/ArchitectureMips.h"
 #include "Plugins/Architecture/PPC64/ArchitecturePPC64.h"
 #include "Plugins/Disassembler/llvm/DisassemblerLLVMC.h"
 #include "Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h"
@@ -326,6 +327,7 @@ SystemInitializerFull::Initialize(const
   ABISysV_s390x::Initialize();
 
   ArchitectureArm::Initialize();
+  ArchitectureMips::Initialize();
   ArchitecturePPC64::Initialize();
 
   DisassemblerLLVMC::Initialize();
@@ -440,6 +442,10 @@ void SystemInitializerFull::Terminate()
 
   ClangASTContext::Terminate();
 
+  ArchitectureArm::Terminate();
+  ArchitectureMips::Terminate();
+  ArchitecturePPC64::Terminate();
+
   ABIMacOSX_i386::Terminate();
   ABIMacOSX_arm::Terminate();
   ABIMacOSX_arm64::Terminate();


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

[Lldb-commits] [PATCH] D55575: [NativePDB] Support local variables

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The test looks really good now.

This looks fine to me, though I don't know much about PBDs. I still think the 
seemingly random change in `BinaryStreamArray` would be better served as a 
standalone patch.




Comment at: lldb/lit/SymbolFile/NativePDB/Inputs/local-variables.lldbinit:19-22
+p Param1
+p Param2
+p Local1
+p Local2

I am not sure if you care about the distinction between "expr" and "frame 
variable" here, but if you don't, these could be displayed in one shot with 
`frame variable Param1 Param2 Local1 Local2` (or even just `frame variable` 
which would display all locals).



Comment at: 
lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:1929
+}
+#include "lldb/Target/MemoryRegionInfo.h"
+CompilerDeclContext

zturner wrote:
> labath wrote:
> > Huh?
> Oops :-/
It looks like you still haven't removed this.


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

https://reviews.llvm.org/D55575



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

[Lldb-commits] [PATCH] D55626: [Reproducers] Add tests for functionality

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 178075.
JDevlieghere marked an inline comment as done.
JDevlieghere added a comment.

Address Adrian's feedback


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

https://reviews.llvm.org/D55626

Files:
  lit/Reproducer/Functionalities/Inputs/DataFormatter.in
  lit/Reproducer/Functionalities/Inputs/foo.cpp
  lit/Reproducer/Functionalities/Inputs/stepping.c
  lit/Reproducer/Functionalities/TestDataFormatter.test
  lit/Reproducer/Functionalities/TestImagineList.test
  lit/Reproducer/Functionalities/TestStepping.test

Index: lit/Reproducer/Functionalities/TestStepping.test
===
--- /dev/null
+++ lit/Reproducer/Functionalities/TestStepping.test
@@ -0,0 +1,100 @@
+# UNSUPPORTED: system-windows, system-freebsd
+
+# This tests that stepping continues to work when replaying a reproducer.
+
+# RUN: rm -rf %T/stepping
+# RUN: %clang %S/Inputs/stepping.c -g -o %t.out
+# RUN: grep -v '#' %s > %t.in
+
+# RUN: %lldb -x -b -s %t.in --capture %T/stepping %t.out | FileCheck %s --check-prefix CHECK --check-prefix CAPTURE
+# RUN: %lldb --replay %T/stepping %t.out | FileCheck %s --check-prefix CHECK --check-prefix REPLAY
+
+# Set breakpoints in a,b and c and verify we stop there when stepping.
+
+breakpoint set -f stepping.c -l 32
+# CHECK: Breakpoint 1: {{.*}} stepping.c:32
+
+breakpoint set -f stepping.c -l 10
+# CHECK: Breakpoint 2: {{.*}} stepping.c:10
+
+breakpoint set -f stepping.c -l 19
+# CHECK: Breakpoint 3: {{.*}} stepping.c:19
+
+breakpoint set -f stepping.c -l 24
+# CHECK: Breakpoint 4: {{.*}} stepping.c:24
+
+run
+# CHECK: Process {{.*}} stopped
+# CHECK: stop reason = breakpoint 1.1
+
+next
+# CHECK: Process {{.*}} stopped
+# CHECK: stop reason = breakpoint 2.1
+
+next
+# CHECK: Process {{.*}} stopped
+# CHECK: stop reason = breakpoint 3.1
+
+next
+# CHECK: Process {{.*}} stopped
+# CHECK: stop reason = breakpoint 4.1
+
+bt
+# CHECK: frame #0: {{.*}}`c
+# CHECK: frame #1: {{.*}}`b
+# CHECK: frame #2: {{.*}}`a
+
+# Continue and stop resulting from the step overs.
+
+cont
+# CHECK: Process {{.*}} resuming
+# CHECK: Process {{.*}} stopped
+# CHECK: stop reason = step over
+cont
+# CHECK: Process {{.*}} resuming
+# CHECK: Process {{.*}} stopped
+# CHECK: stop reason = step over
+cont
+# CHECK: Process {{.*}} resuming
+# CHECK: Process {{.*}} stopped
+# CHECK: stop reason = step over
+
+breakpoint disable 1
+# CHECK: 1 breakpoints disabled.
+
+breakpoint disable 2
+# CHECK: 1 breakpoints disabled.
+
+breakpoint disable 3
+# CHECK: 1 breakpoints disabled.
+
+breakpoint disable 4
+# CHECK: 1 breakpoints disabled.
+
+# Continue to line 48.
+
+next
+# CHECK: stop reason = step over
+next
+# CHECK: stop reason = step over
+
+# Step into c.
+thread step-in
+# CHECK: stop reason = step in
+thread step-in
+# CHECK: stop reason = step in
+thread step-in
+# CHECK: stop reason = step in
+thread step-in
+# CHECK: stop reason = step in
+thread step-in
+# CHECK: stop reason = step in
+
+# Finally continue until the end.
+
+cont
+# CHECK: Process {{.*}} resuming
+# CHECK: Process {{.*}} exited with status = 0
+
+# Generate the reproducer.
+reproducer generate
Index: lit/Reproducer/Functionalities/TestImagineList.test
===
--- /dev/null
+++ lit/Reproducer/Functionalities/TestImagineList.test
@@ -0,0 +1,32 @@
+# UNSUPPORTED: system-windows, system-freebsd
+
+# This tests that image list works when replaying. We arbitrarily assume
+# there's at least two entries and compare that they're identical.
+
+# RUN: %clang %S/Inputs/stepping.c -g -o %t.out
+
+# RUN: rm -rf %T/imagelist
+# RUN: rm -rf %t.txt
+
+# RUN: echo "CAPTURE" >> %t.txt
+# RUN: %lldb -x -b  --capture %T/imagelist \
+# RUN:-o 'image list' \
+# RUN:-o 'reproducer generate' \
+# RUN:%t.out >> %t.txt 2>&1
+
+# RUN: rm -rf
+
+# RUN: echo "REPLAY" >> %t.txt
+# RUN: %lldb -x -b --replay %T/imagelist  %t.out >> %t.txt 2>&1
+
+# RUN: cat %t.txt | FileCheck %s
+
+# CHECK: CAPTURE
+# CHECK: image list
+# CHECK: [  0] [[ZERO:.*]]
+# CHECK: [  1] [[ONE:.*]]
+
+# CHECK: REPLAY
+# CHECK: image list
+# CHECK: [  0] {{.*}}[[ZERO]]
+# CHECK: [  1] {{.*}}[[ONE]]
Index: lit/Reproducer/Functionalities/TestDataFormatter.test
===
--- /dev/null
+++ lit/Reproducer/Functionalities/TestDataFormatter.test
@@ -0,0 +1,16 @@
+# UNSUPPORTED: system-windows, system-freebsd
+
+# This tests that data formatters continue to work when replaying a reproducer.
+
+# RUN: rm -rf %T/dataformatter
+# RUN: %clang -x c++ %S/Inputs/foo.cpp -g -o %t.out
+
+# RUN: %lldb -x -b -s %S/Inputs/DataFormatter.in --capture %T/dataformatter %t.out | FileCheck %s
+# RUN: %lldb --replay %T/dataformatter %t.out | FileCheck %s
+
+# CHECK: stop reason = breakpoint 1.1
+# CHECK: (Foo) foo = (m_i = 0, m_d = 0)
+# CHECK: stop reason = step over
+# CHECK: (Foo) foo = (m_i = 1, m_

[Lldb-commits] [PATCH] D55626: [Reproducers] Add tests for functionality

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked 4 inline comments as done.
JDevlieghere added inline comments.



Comment at: lit/Reproducer/Functionalities/Inputs/stepping.c:8
+//
+//===--===//
+#include 

aprantl wrote:
> Why does only this file have a header?
I initially copied the file from the dotest suite before modifying it :-) 


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

https://reviews.llvm.org/D55626



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

[Lldb-commits] [PATCH] D55582: [Reproducers] Add command reproducer

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D55582#1329479 , @labath wrote:

> I am very sorry about this, but I am having some second thoughts about the 
> `RunReplay` thingy. I don't think I've correctly expressed what is bothering 
> me about it (and that's probably because I wasn't clear with myself about 
> what is bothering me). I'll try to formulate it better this time


No worries, I appreciate you thinking this over :-)

> What I was thinking about were the boundaries at which we want to 
> record/replay events. If I understand correctly, one of those boundaries 
> should be the SB layer (is that correct?). So, with that in mind, it makes 
> sense for the lldb driver to do something "special" in replay mode, and 
> invoke some other function which would replay the SB calls it would have 
> normally made. That function might as well be called `RunReplay`.

Yep, the SB layer is an important boundary. Everything that happens in Xcode 
goes through it. It's probably going to be one of the harder parts of the 
feature. Every API call needs to be able to  capture and replay while keeping 
things like references consistent. We have spent some time thinking about this, 
but I'm sure a bunch of complexities will pop up once we start doing the actual 
work.

> Now, if we imagine we have this SB record/replay capability, during recording 
> it would detect that the driver made a call to 
> `SBDebugger::RunCommandInterpreter`. Then, during recording it would again 
> make a call to SBDebugger::RunCommandInterpreter, but ensure that it behaves 
> the same way as it did the first time (this is the auto-detection part I was 
> speaking about).

I see what you mean now, and it's a fair point/concern. So far I've been 
postponing dealing with the SB layer until I could get reproducers to work with 
the command driver. Basically I'll have to audit all the SB endpoints anyway so 
I figured patching this up was going to be negligible compared to that effort. 
However, you're totally right that we shouldn't ignore its impact on design 
decisions. Moving it into a separate function will make it easier down the 
road, as we can just do "the right" thing in replay mode for that particular 
function.

> The reason this came out all wrong is because I was also thinking about a 
> (somewhat) unrelated thing, and that is the layer at which we do the 
> capturing of the commands. I was thinking that maybe the command interpreter 
> doesn't need to know anything about the record/replay taking place. If this 
> SB replay engine substituted the `stdin` of the debugger with a recorded 
> version of it, then it could go normally go about it's business and things 
> would "just work" (you'd still have to make sure that libedit doesn't do 
> something funny, but that could hopefully be done at the IOHandler level by 
> substituting the editline IO handler).

I didn't consider that idea but I like the sound of it :-)

> So, I mean, that's how I'd do it, but this is your feature, and where you 
> wan't to draw the record/replay line ultimately depends on what you want to 
> get out of the feature. However, I do see some advantages to doing the r/r at 
> a lower level that I'd like to try to sell to you:
> 
> - better possibility for capturing ^C. I think ^C is fairly important, 
> particularly in console debugging, as it's the only way to interrupt a 
> program without it voluntarily hitting a breakpoint. It will also be fairly 
> tricky to reproduce. With the current framework, I'm not sure how you would 
> manage to capture that. If you worked with IO handlers and was capturing 
> stdin, I think you'd have a better chance of getting it right (since both 
> stdin and ^C come from the terminal).
> - no need for special handling of command sourcing. As you would capture 
> almost-raw stdin, you wouldn't need to do anything special to the avoid 
> sourced commands being captured twice. The filesystem would just provide the 
> captured file, and stdin-recorder would capture the stdin. (which I think 
> would make the stdin-capturer fit nicely with the Filesystem one, as they're 
> both fairly low-level).
> - right now you're merging commands coming from multiple sources (stdin via 
> `RunCommandInterpreter`, command strings from `HandleCommand`) into one 
> monolithic command stream. I think this means that once you have an SB 
> recorder, it will also have to special case command APIs to avoid doing 
> things twice. If you work at a lower level, this won't happen, because 
> HandleCommand would be naturally captured as part of the SB stream, and stdin 
> commands would be captured by the stdin-recorder (which would probably also 
> be initialized and managed by the SB recorder, since it needs to be hooked 
> into RunCommandInterpreter)

+1 on all these points. I'll look into doing it this way today.

> So, basically, my point is:
> 
> - let's bring back `RunReplay`. I don't think it makes sense to force th

[Lldb-commits] [lldb] r349062 - Fix MinidumpParser::GetFilteredModuleList() and test it

[Greg Clayton via lldb-commits]

Author: gclayton
Date: Thu Dec 13 09:24:30 2018
New Revision: 349062

URL: http://llvm.org/viewvc/llvm-project?rev=349062&view=rev
Log:
Fix MinidumpParser::GetFilteredModuleList() and test it

The MinidumpParser::GetFilteredModuleList() code was attempting to iterate 
through the entire module list and if it found more than one entry for a given 
module name, it wanted to pick the MinidumpModule with the lowest address. A 
bug existed where it wasn't doing that due to "exists" variable being inverted. 
"exists" was set to true if it was inserted, not if it existed. Furthermore, 
the order of the modules would be modified by sorting all modules from low 
address to high address (using MinidumpModule::base_of_image). This fix also 
maintains the original order which means your executable is at index 0 as 
intended instead of some random shared library.

Tests were added to ensure this functionality doesn't regress.

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


Added:
lldb/trunk/unittests/Process/minidump/Inputs/modules-dup-min-addr.dmp   
(with props)
lldb/trunk/unittests/Process/minidump/Inputs/modules-order.dmp   (with 
props)
Modified:
lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp
lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp

Modified: lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp?rev=349062&r1=349061&r2=349062&view=diff
==
--- lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp (original)
+++ lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp Thu Dec 13 
09:24:30 2018
@@ -274,36 +274,45 @@ llvm::ArrayRef MinidumpP
 
 std::vector MinidumpParser::GetFilteredModuleList() {
   llvm::ArrayRef modules = GetModuleList();
-  // map module_name -> pair(load_address, pointer to module struct in memory)
-  llvm::StringMap> lowest_addr;
+  // map module_name -> filtered_modules index
+  typedef llvm::StringMap MapType;
+  MapType module_name_to_filtered_index;
 
   std::vector filtered_modules;
-
+  
   llvm::Optional name;
   std::string module_name;
 
   for (const auto &module : modules) {
 name = GetMinidumpString(module.module_name_rva);
-
+
 if (!name)
   continue;
-
+
 module_name = name.getValue();
-
-auto iter = lowest_addr.end();
-bool exists;
-std::tie(iter, exists) = lowest_addr.try_emplace(
-module_name, std::make_pair(module.base_of_image, &module));
-
-if (exists && module.base_of_image < iter->second.first)
-  iter->second = std::make_pair(module.base_of_image, &module);
-  }
-
-  filtered_modules.reserve(lowest_addr.size());
-  for (const auto &module : lowest_addr) {
-filtered_modules.push_back(module.second.second);
+
+MapType::iterator iter;
+bool inserted;
+// See if we have inserted this module aready into filtered_modules. If we
+// haven't insert an entry into module_name_to_filtered_index with the
+// index where we will insert it if it isn't in the vector already.
+std::tie(iter, inserted) = module_name_to_filtered_index.try_emplace(
+module_name, filtered_modules.size());
+
+if (inserted) {
+  // This module has not been seen yet, insert it into filtered_modules at
+  // the index that was inserted into module_name_to_filtered_index using
+  // "filtered_modules.size()" above.
+  filtered_modules.push_back(&module);
+} else {
+  // This module has been seen. Modules are sometimes mentioned multiple
+  // times when they are mapped discontiguously, so find the module with
+  // the lowest "base_of_image" and use that as the filtered module.
+  auto dup_module = filtered_modules[iter->second];
+  if (module.base_of_image < dup_module->base_of_image)
+filtered_modules[iter->second] = &module;
+}
   }
-
   return filtered_modules;
 }
 

Added: lldb/trunk/unittests/Process/minidump/Inputs/modules-dup-min-addr.dmp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Process/minidump/Inputs/modules-dup-min-addr.dmp?rev=349062&view=auto
==
Binary file - no diff available.

Propchange: 
lldb/trunk/unittests/Process/minidump/Inputs/modules-dup-min-addr.dmp
--
svn:mime-type = application/octet-stream

Added: lldb/trunk/unittests/Process/minidump/Inputs/modules-order.dmp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Process/minidump/Inputs/modules-order.dmp?rev=349062&view=auto
==
Binary file - no diff available.

Propchange: lldb/trunk/unittests/Process/minidump/Inputs/modules-order.dmp
---

[Lldb-commits] [PATCH] D55614: Fix MinidumpParser::GetFilteredModuleList() and test it

[Phabricator via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL349062: Fix MinidumpParser::GetFilteredModuleList() and test 
it (authored by gclayton, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55614?vs=177902&id=178086#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55614

Files:
  lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp
  lldb/trunk/unittests/Process/minidump/Inputs/modules-dup-min-addr.dmp
  lldb/trunk/unittests/Process/minidump/Inputs/modules-order.dmp
  lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp

Index: lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp
===
--- lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp
+++ lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp
@@ -533,3 +533,41 @@
 }
   }
 }
+
+TEST_F(MinidumpParserTest, MinidumpDuplicateModuleMinAddress) {
+  SetUpData("modules-dup-min-addr.dmp");
+  // Test that if we have two modules in the module list:
+  ///tmp/a with range [0x2000-0x3000)
+  ///tmp/a with range [0x1000-0x2000)
+  // That we end up with one module in the filtered list with the
+  // range [0x1000-0x2000). MinidumpParser::GetFilteredModuleList() is
+  // trying to ensure that if we have the same module mentioned more than
+  // one time, we pick the one with the lowest base_of_image.
+  std::vector filtered_modules =
+  parser->GetFilteredModuleList();
+  EXPECT_EQ(1, filtered_modules.size());
+  EXPECT_EQ(0x1000, filtered_modules[0]->base_of_image);
+}
+
+TEST_F(MinidumpParserTest, MinidumpModuleOrder) {
+  SetUpData("modules-order.dmp");
+  // Test that if we have two modules in the module list:
+  ///tmp/a with range [0x2000-0x3000)
+  ///tmp/b with range [0x1000-0x2000)
+  // That we end up with two modules in the filtered list with the same ranges
+  // and in the same order. Previous versions of the
+  // MinidumpParser::GetFilteredModuleList() function would sort all images
+  // by address and modify the order of the modules.
+  std::vector filtered_modules =
+  parser->GetFilteredModuleList();
+  llvm::Optional name;
+  EXPECT_EQ(2, filtered_modules.size());
+  EXPECT_EQ(0x2000, filtered_modules[0]->base_of_image);
+  name = parser->GetMinidumpString(filtered_modules[0]->module_name_rva);
+  ASSERT_TRUE((bool)name);
+  EXPECT_EQ(std::string("/tmp/a"), *name);
+  EXPECT_EQ(0x1000, filtered_modules[1]->base_of_image);
+  name = parser->GetMinidumpString(filtered_modules[1]->module_name_rva);
+  ASSERT_TRUE((bool)name);
+  EXPECT_EQ(std::string("/tmp/b"), *name);
+}
Index: lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp
===
--- lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp
+++ lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp
@@ -274,36 +274,45 @@
 
 std::vector MinidumpParser::GetFilteredModuleList() {
   llvm::ArrayRef modules = GetModuleList();
-  // map module_name -> pair(load_address, pointer to module struct in memory)
-  llvm::StringMap> lowest_addr;
+  // map module_name -> filtered_modules index
+  typedef llvm::StringMap MapType;
+  MapType module_name_to_filtered_index;
 
   std::vector filtered_modules;
-
+  
   llvm::Optional name;
   std::string module_name;
 
   for (const auto &module : modules) {
 name = GetMinidumpString(module.module_name_rva);
-
+
 if (!name)
   continue;
-
+
 module_name = name.getValue();
-
-auto iter = lowest_addr.end();
-bool exists;
-std::tie(iter, exists) = lowest_addr.try_emplace(
-module_name, std::make_pair(module.base_of_image, &module));
-
-if (exists && module.base_of_image < iter->second.first)
-  iter->second = std::make_pair(module.base_of_image, &module);
-  }
-
-  filtered_modules.reserve(lowest_addr.size());
-  for (const auto &module : lowest_addr) {
-filtered_modules.push_back(module.second.second);
+
+MapType::iterator iter;
+bool inserted;
+// See if we have inserted this module aready into filtered_modules. If we
+// haven't insert an entry into module_name_to_filtered_index with the
+// index where we will insert it if it isn't in the vector already.
+std::tie(iter, inserted) = module_name_to_filtered_index.try_emplace(
+module_name, filtered_modules.size());
+
+if (inserted) {
+  // This module has not been seen yet, insert it into filtered_modules at
+  // the index that was inserted into module_name_to_filtered_index using
+  // "filtered_modules.size()" above.
+  filtered_modules.push_back(&module);
+} else {
+  // This module has been seen. Modules are sometimes mentioned multiple
+  // times when they are mapped disc

[Lldb-commits] [PATCH] D55631: Delay replacing the process till after we've finalized it

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: source/Target/Target.cpp:2867-2880
 // Get a weak pointer to the previous process if we have one
 ProcessWP process_wp;
 if (m_process_sp)
   process_wp = m_process_sp;
-m_process_sp =
-GetPlatform()->DebugProcess(launch_info, debugger, this, error);
 
 // Cleanup the old process since someone might still have a strong
 // reference to this process and we would like to allow it to cleanup as

All this weak pointer code is no longer needed now? This code:

```
// Get a weak pointer to the previous process if we have one
ProcessWP process_wp;
if (m_process_sp)
  process_wp = m_process_sp;



// Cleanup the old process since someone might still have a strong
// reference to this process and we would like to allow it to cleanup as
// much as it can without the object being destroyed. We try to lock the
// shared pointer and if that works, then someone else still has a strong
// reference to the process.

ProcessSP old_process_sp(process_wp.lock());
if (old_process_sp)
  old_process_sp->Finalize();
```

Will just become:

```
if (m_process_sp)
  m_process_sp->Finalize();
```

The original thinking was that Process::Finalize() was just an optional call 
that could be made in case a process does not go away to reduce the memory that 
it owned in case it sticks around forever and also to mark the process in a way 
that would stop it from doing any future work (don't try to fetch thread lists, 
etc). Has Finalize changed to something that is required now? Are we doing work 
that must be done in order to properly shut down a process? That wasn't the 
original intention IIRC. "svn blame" was no help as all this code was there 
prior the great reformatting of code.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55631



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

[Lldb-commits] [lldb] r349067 - [NativePDB] Add support for local variables.

[Zachary Turner via lldb-commits]

Author: zturner
Date: Thu Dec 13 10:17:51 2018
New Revision: 349067

URL: http://llvm.org/viewvc/llvm-project?rev=349067&view=rev
Log:
[NativePDB] Add support for local variables.

This patch adds support for parsing and evaluating local variables.
using the native pdb plugin.

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

Added:
lldb/trunk/lit/SymbolFile/NativePDB/Inputs/local-variables.lldbinit
lldb/trunk/lit/SymbolFile/NativePDB/local-variables.cpp
Modified:
lldb/trunk/source/Plugins/SymbolFile/NativePDB/DWARFLocationExpression.cpp
lldb/trunk/source/Plugins/SymbolFile/NativePDB/DWARFLocationExpression.h
lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbSymUid.h
lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
lldb/trunk/source/Symbol/ClangASTContext.cpp

Added: lldb/trunk/lit/SymbolFile/NativePDB/Inputs/local-variables.lldbinit
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/SymbolFile/NativePDB/Inputs/local-variables.lldbinit?rev=349067&view=auto
==
--- lldb/trunk/lit/SymbolFile/NativePDB/Inputs/local-variables.lldbinit (added)
+++ lldb/trunk/lit/SymbolFile/NativePDB/Inputs/local-variables.lldbinit Thu Dec 
13 10:17:51 2018
@@ -0,0 +1,32 @@
+break set -n main
+run a b c d e f g
+p argc
+step
+p SomeLocal
+step
+p Param1
+p Param2
+step
+p Param1
+p Param2
+p Local1
+step
+p Param1
+p Param2
+p Local1
+p Local2
+step
+p Param1
+p Param2
+p Local1
+p Local2
+step
+p Param1
+p Param2
+p Local1
+p Local2
+continue
+
+target modules dump ast
+
+quit

Added: lldb/trunk/lit/SymbolFile/NativePDB/local-variables.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/SymbolFile/NativePDB/local-variables.cpp?rev=349067&view=auto
==
--- lldb/trunk/lit/SymbolFile/NativePDB/local-variables.cpp (added)
+++ lldb/trunk/lit/SymbolFile/NativePDB/local-variables.cpp Thu Dec 13 10:17:51 
2018
@@ -0,0 +1,161 @@
+// clang-format off
+
+// REQUIRES: system-windows
+// RUN: %build -o %t.exe -- %s
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t.exe -s \
+// RUN: %p/Inputs/local-variables.lldbinit 2>&1 | FileCheck %s
+
+int Function(int Param1, char Param2) {
+  unsigned Local1 = Param1 + 1;
+  char Local2 = Param2 + 1;
+  ++Local1;
+  ++Local2;
+  return Local1;
+}
+
+int main(int argc, char **argv) {
+  int SomeLocal = argc * 2;
+  return Function(SomeLocal, 'a');
+}
+
+// CHECK:  (lldb) target create "{{.*}}local-variables.cpp.tmp.exe"
+// CHECK-NEXT: Current executable set to '{{.*}}local-variables.cpp.tmp.exe'
+// CHECK-NEXT: (lldb) command source -s 0 '{{.*}}local-variables.lldbinit'
+// CHECK-NEXT: Executing commands in '{{.*}}local-variables.lldbinit'.
+// CHECK-NEXT: (lldb) break set -n main
+// CHECK-NEXT: Breakpoint 1: where = local-variables.cpp.tmp.exe`main + {{.*}} 
at local-variables.cpp:{{.*}}, address = {{.*}}
+// CHECK-NEXT: (lldb) run a b c d e f g
+// CHECK-NEXT: Process {{.*}} stopped
+// CHECK-NEXT: * thread #1, stop reason = breakpoint 1.1
+// CHECK-NEXT: frame #0: {{.*}} local-variables.cpp.tmp.exe`main(argc=8, 
argv={{.*}}) at local-variables.cpp:{{.*}}
+// CHECK-NEXT:14   }
+// CHECK-NEXT:15
+// CHECK-NEXT:16   int main(int argc, char **argv) {
+// CHECK-NEXT: -> 17 int SomeLocal = argc * 2;
+// CHECK-NEXT:18 return Function(SomeLocal, 'a');
+// CHECK-NEXT:19   }
+// CHECK-NEXT:20
+
+// CHECK:  Process {{.*}} launched: '{{.*}}local-variables.cpp.tmp.exe'
+// CHECK-NEXT: (lldb) p argc
+// CHECK-NEXT: (int) $0 = 8
+// CHECK-NEXT: (lldb) step
+// CHECK-NEXT: Process {{.*}} stopped
+// CHECK-NEXT: * thread #1, stop reason = step in
+// CHECK-NEXT: frame #0: {{.*}} local-variables.cpp.tmp.exe`main(argc=8, 
argv={{.*}}) at local-variables.cpp:{{.*}}
+// CHECK-NEXT:15
+// CHECK-NEXT:16 int main(int argc, char **argv) {
+// CHECK-NEXT:17 int SomeLocal = argc * 2;
+// CHECK-NEXT: -> 18 return Function(SomeLocal, 'a');
+// CHECK-NEXT:19 }
+// CHECK-NEXT:20
+
+// CHECK:  (lldb) p SomeLocal
+// CHECK-NEXT: (int) $1 = 16
+// CHECK-NEXT: (lldb) step
+// CHECK-NEXT: Process {{.*}} stopped
+// CHECK-NEXT: * thread #1, stop reason = step in
+// CHECK-NEXT: frame #0: {{.*}} 
local-variables.cpp.tmp.exe`Function(Param1=16, Param2='a') at 
local-variables.cpp:{{.*}}
+// CHECK-NEXT:6
+// CHECK-NEXT:7
+// CHECK-NEXT:8 int Function(int Param1, char Param2) {
+// CHECK-NEXT: -> 9  unsigned Local1 = Param1 + 1;
+// CHECK-NEXT:10 char Local2 = Param2 + 1;
+// CHECK-NEXT:11 ++Local1;
+// CHECK-NEXT:12 ++Local2;
+
+// CHECK:  (lldb) p Param1
+// CHECK-NEXT: (int) $2 = 16
+// CHECK-NEXT: (lldb) p Param2
+// CHECK-NEXT: (char) $3 = 'a'
+// CHECK-NEXT: (lldb) step
+// CHECK-NEXT: Process {{.*}} stopped
+

[Lldb-commits] [PATCH] D55575: [NativePDB] Support local variables

[Zachary Turner via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rLLDB349067: [NativePDB] Add support for local variables. 
(authored by zturner, committed by ).
Herald added subscribers: teemperor, abidh.

Changed prior to commit:
  https://reviews.llvm.org/D55575?vs=177877&id=178090#toc

Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55575

Files:
  lit/SymbolFile/NativePDB/Inputs/local-variables.lldbinit
  lit/SymbolFile/NativePDB/local-variables.cpp
  source/Plugins/SymbolFile/NativePDB/DWARFLocationExpression.cpp
  source/Plugins/SymbolFile/NativePDB/DWARFLocationExpression.h
  source/Plugins/SymbolFile/NativePDB/PdbSymUid.h
  source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
  source/Symbol/ClangASTContext.cpp

Index: source/Symbol/ClangASTContext.cpp
===
--- source/Symbol/ClangASTContext.cpp
+++ source/Symbol/ClangASTContext.cpp
@@ -2218,10 +2218,13 @@
 const CompilerType ¶m_type, int storage) {
   ASTContext *ast = getASTContext();
   assert(ast != nullptr);
-  return ParmVarDecl::Create(*ast, decl_ctx, SourceLocation(), SourceLocation(),
- name && name[0] ? &ast->Idents.get(name) : nullptr,
- ClangUtil::GetQualType(param_type), nullptr,
- (clang::StorageClass)storage, nullptr);
+  auto *decl =
+  ParmVarDecl::Create(*ast, decl_ctx, SourceLocation(), SourceLocation(),
+  name && name[0] ? &ast->Idents.get(name) : nullptr,
+  ClangUtil::GetQualType(param_type), nullptr,
+  (clang::StorageClass)storage, nullptr);
+  decl_ctx->addDecl(decl);
+  return decl;
 }
 
 void ClangASTContext::SetFunctionParameters(FunctionDecl *function_decl,
Index: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
===
--- source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
+++ source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
@@ -109,10 +109,10 @@
VariableList &variables) override;
 
   size_t ParseTypes(const SymbolContext &sc) override;
-  size_t ParseVariablesForContext(const SymbolContext &sc) override {
-return 0;
-  }
+  size_t ParseVariablesForContext(const SymbolContext &sc) override;
 
+  CompilerDecl GetDeclForUID(lldb::user_id_t uid) override;
+  CompilerDeclContext GetDeclContextForUID(lldb::user_id_t uid) override;
   CompilerDeclContext GetDeclContextContainingUID(lldb::user_id_t uid) override;
   Type *ResolveTypeUID(lldb::user_id_t type_uid) override;
   llvm::Optional GetDynamicArrayInfoForUID(
@@ -194,20 +194,30 @@
  clang::MSInheritanceAttr::Spelling inheritance);
 
   lldb::FunctionSP GetOrCreateFunction(PdbCompilandSymId func_id,
-   const SymbolContext &sc);
+   CompileUnit &comp_unit);
   lldb::CompUnitSP GetOrCreateCompileUnit(const CompilandIndexItem &cci);
   lldb::TypeSP GetOrCreateType(PdbTypeSymId type_id);
   lldb::TypeSP GetOrCreateType(llvm::codeview::TypeIndex ti);
   lldb::VariableSP GetOrCreateGlobalVariable(PdbGlobalSymId var_id);
+  Block &GetOrCreateBlock(PdbCompilandSymId block_id);
+  lldb::VariableSP GetOrCreateLocalVariable(PdbCompilandSymId scope_id,
+PdbCompilandSymId var_id,
+bool is_param);
 
   lldb::FunctionSP CreateFunction(PdbCompilandSymId func_id,
-  const SymbolContext &sc);
+  CompileUnit &comp_unit);
+  Block &CreateBlock(PdbCompilandSymId block_id);
+  lldb::VariableSP CreateLocalVariable(PdbCompilandSymId scope_id,
+   PdbCompilandSymId var_id, bool is_param);
   lldb::CompUnitSP CreateCompileUnit(const CompilandIndexItem &cci);
   lldb::TypeSP CreateType(PdbTypeSymId type_id);
   lldb::TypeSP CreateAndCacheType(PdbTypeSymId type_id);
   lldb::VariableSP CreateGlobalVariable(PdbGlobalSymId var_id);
   lldb::VariableSP CreateConstantSymbol(PdbGlobalSymId var_id,
 const llvm::codeview::CVSymbol &cvs);
+  size_t ParseVariablesForCompileUnit(CompileUnit &comp_unit,
+  VariableList &variables);
+  size_t ParseVariablesForBlock(PdbCompilandSymId block_id);
 
   llvm::BumpPtrAllocator m_allocator;
 
@@ -224,6 +234,8 @@
   m_parent_types;
 
   llvm::DenseMap m_global_vars;
+  llvm::DenseMap m_local_variables;
+  llvm::DenseMap m_blocks;
   llvm::DenseMap m_functions;
   llvm::DenseMap m_compilands;
   llvm::DenseMap m_types;
Index:

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via lldb-commits]

What's the consensus?

Personally I think that, even considering the potential issue that Paval
pointed out, the "target symbols add ..." is the most conservative approach
in terms of introducing new behavior. I'm fine with the current directory
lookup as well (the original change) since it's consistent with DWARF
lookup.
(the only choice I'm less excited about is the implicit lookup next to the
.dmp or .dll/.exe - it's highly specific to a local debugging scenario
which may be appropriate for something like an IDE, ex. VisualStudio but
not for a general purpose debugger)

So my preference is for the latest patch - what does everyone else think?

On Thu, Dec 13, 2018 at 4:03 AM Zachary Turner  wrote:

> On irc earlier i was talking about this with Greg and he said it should be
> fine in his opinion. I’ll point him to this review in the morning so he can
> comment
> On Thu, Dec 13, 2018 at 3:30 AM Pavel Labath via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> labath added inline comments.
>>
>>
>> 
>> Comment at: source/Commands/CommandObjectTarget.cpp:4246
>>if (symbol_file) {
>> -ObjectFile *object_file = symbol_file->GetObjectFile();
>>
>> 
>> lemo wrote:
>> > note I had to bypass this check: we don't (yet) have a ObjectFilePDB so
>> the SymbolFileNativePDB always points to the associated PE binary.
>> >
>> > the check itself seems valuable as a diagnostic but not strictly
>> required. Should I add a TODO comment and/or open a bug to revisit this?
>> I not sure this is a good idea. Isn't this the only way of providing
>> feedback about whether the symbols were actually added? If we are unable to
>> load the symbol file specified (perhaps because the user made a typo, or
>> the file is corrupted), then the symbol vendor will just create a default
>> SymbolFile backed by the original object file. Doesn't that mean this will
>> basically always return true now?
>>
>> I think this is strictly worse that the previous solution as it lets the
>> objectless-symbol-file hack leak out of SymbolFilePDB.
>>
>>
>> CHANGES SINCE LAST ACTION
>>   https://reviews.llvm.org/D55142/new/
>>
>> https://reviews.llvm.org/D55142
>>
>>
>>
>>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via lldb-commits]

Well, Visual Studio also supports remote debugging, and searching next to
the .dmp is just one of several places it looks.  And LLDB also supports
local debugging, and so looking next to the .dmp file, being consistent
with Visual Studio, will be the expected behavior for people using LLDB in
local debugging scenarios, which is one of the supported use cases.

That said, I'm also fine with the latest patch.

On Thu, Dec 13, 2018 at 10:32 AM Leonard Mosescu  wrote:

> What's the consensus?
>
> Personally I think that, even considering the potential issue that Paval
> pointed out, the "target symbols add ..." is the most conservative approach
> in terms of introducing new behavior. I'm fine with the current directory
> lookup as well (the original change) since it's consistent with DWARF
> lookup.
> (the only choice I'm less excited about is the implicit lookup next to the
> .dmp or .dll/.exe - it's highly specific to a local debugging scenario
> which may be appropriate for something like an IDE, ex. VisualStudio but
> not for a general purpose debugger)
>
> So my preference is for the latest patch - what does everyone else think?
>
> On Thu, Dec 13, 2018 at 4:03 AM Zachary Turner  wrote:
>
>> On irc earlier i was talking about this with Greg and he said it should
>> be fine in his opinion. I’ll point him to this review in the morning so he
>> can comment
>> On Thu, Dec 13, 2018 at 3:30 AM Pavel Labath via Phabricator <
>> revi...@reviews.llvm.org> wrote:
>>
>>> labath added inline comments.
>>>
>>>
>>> 
>>> Comment at: source/Commands/CommandObjectTarget.cpp:4246
>>>if (symbol_file) {
>>> -ObjectFile *object_file = symbol_file->GetObjectFile();
>>>
>>> 
>>> lemo wrote:
>>> > note I had to bypass this check: we don't (yet) have a ObjectFilePDB
>>> so the SymbolFileNativePDB always points to the associated PE binary.
>>> >
>>> > the check itself seems valuable as a diagnostic but not strictly
>>> required. Should I add a TODO comment and/or open a bug to revisit this?
>>> I not sure this is a good idea. Isn't this the only way of providing
>>> feedback about whether the symbols were actually added? If we are unable to
>>> load the symbol file specified (perhaps because the user made a typo, or
>>> the file is corrupted), then the symbol vendor will just create a default
>>> SymbolFile backed by the original object file. Doesn't that mean this will
>>> basically always return true now?
>>>
>>> I think this is strictly worse that the previous solution as it lets the
>>> objectless-symbol-file hack leak out of SymbolFilePDB.
>>>
>>>
>>> CHANGES SINCE LAST ACTION
>>>   https://reviews.llvm.org/D55142/new/
>>>
>>> https://reviews.llvm.org/D55142
>>>
>>>
>>>
>>>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55116: [CMake] llvm_codesign workaround for Xcode double-signing errors

[Phabricator via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL349070: [CMake] llvm_codesign workaround for Xcode 
double-signing errors (authored by stefan.graenitz, committed by ).
Herald added a subscriber: michaelplatings.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55116

Files:
  llvm/trunk/cmake/modules/AddLLVM.cmake


Index: llvm/trunk/cmake/modules/AddLLVM.cmake
===
--- llvm/trunk/cmake/modules/AddLLVM.cmake
+++ llvm/trunk/cmake/modules/AddLLVM.cmake
@@ -584,7 +584,7 @@
 
   if(ARG_SHARED OR ARG_MODULE)
 llvm_externalize_debuginfo(${name})
-llvm_codesign(TARGET ${name})
+llvm_codesign(${name})
   endif()
 endfunction()
 
@@ -796,7 +796,7 @@
 target_link_libraries(${name} PRIVATE ${LLVM_PTHREAD_LIB})
   endif()
 
-  llvm_codesign(TARGET ${name} ENTITLEMENTS ${ARG_ENTITLEMENTS})
+  llvm_codesign(${name} ENTITLEMENTS ${ARG_ENTITLEMENTS})
 endmacro(add_llvm_executable name)
 
 function(export_executable_symbols target)
@@ -1635,13 +1635,9 @@
   endif()
 endfunction()
 
-# Usage: llvm_codesign(TARGET name [ENTITLEMENTS file])
-#
-# Code-sign the given TARGET with the global LLVM_CODESIGNING_IDENTITY or skip
-# if undefined. Customize capabilities by passing a file path to ENTITLEMENTS.
-#
-function(llvm_codesign)
-  cmake_parse_arguments(ARG "" "TARGET;ENTITLEMENTS" "" ${ARGN})
+# Usage: llvm_codesign(name [ENTITLEMENTS file])
+function(llvm_codesign name)
+  cmake_parse_arguments(ARG "" "ENTITLEMENTS" "" ${ARGN})
 
   if(NOT LLVM_CODESIGNING_IDENTITY)
 return()
@@ -1659,15 +1655,20 @@
   )
 endif()
 if(DEFINED ARG_ENTITLEMENTS)
-  set(PASS_ENTITLEMENTS --entitlements ${ARG_ENTITLEMENTS})
+  set(pass_entitlements --entitlements ${ARG_ENTITLEMENTS})
+endif()
+if(CMAKE_GENERATOR STREQUAL "Xcode")
+  # Avoid double-signing error: Since output overwrites input, Xcode runs
+  # the post-build rule even if the actual build-step was skipped.
+  set(pass_force --force)
 endif()
 
 add_custom_command(
-  TARGET ${ARG_TARGET} POST_BUILD
+  TARGET ${name} POST_BUILD
   COMMAND ${CMAKE_COMMAND} -E
   env CODESIGN_ALLOCATE=${CMAKE_CODESIGN_ALLOCATE}
   ${CMAKE_CODESIGN} -s ${LLVM_CODESIGNING_IDENTITY}
-  ${PASS_ENTITLEMENTS} $
+  ${pass_entitlements} ${pass_force} $
 )
   endif()
 endfunction()


Index: llvm/trunk/cmake/modules/AddLLVM.cmake
===
--- llvm/trunk/cmake/modules/AddLLVM.cmake
+++ llvm/trunk/cmake/modules/AddLLVM.cmake
@@ -584,7 +584,7 @@
 
   if(ARG_SHARED OR ARG_MODULE)
 llvm_externalize_debuginfo(${name})
-llvm_codesign(TARGET ${name})
+llvm_codesign(${name})
   endif()
 endfunction()
 
@@ -796,7 +796,7 @@
 target_link_libraries(${name} PRIVATE ${LLVM_PTHREAD_LIB})
   endif()
 
-  llvm_codesign(TARGET ${name} ENTITLEMENTS ${ARG_ENTITLEMENTS})
+  llvm_codesign(${name} ENTITLEMENTS ${ARG_ENTITLEMENTS})
 endmacro(add_llvm_executable name)
 
 function(export_executable_symbols target)
@@ -1635,13 +1635,9 @@
   endif()
 endfunction()
 
-# Usage: llvm_codesign(TARGET name [ENTITLEMENTS file])
-#
-# Code-sign the given TARGET with the global LLVM_CODESIGNING_IDENTITY or skip
-# if undefined. Customize capabilities by passing a file path to ENTITLEMENTS.
-#
-function(llvm_codesign)
-  cmake_parse_arguments(ARG "" "TARGET;ENTITLEMENTS" "" ${ARGN})
+# Usage: llvm_codesign(name [ENTITLEMENTS file])
+function(llvm_codesign name)
+  cmake_parse_arguments(ARG "" "ENTITLEMENTS" "" ${ARGN})
 
   if(NOT LLVM_CODESIGNING_IDENTITY)
 return()
@@ -1659,15 +1655,20 @@
   )
 endif()
 if(DEFINED ARG_ENTITLEMENTS)
-  set(PASS_ENTITLEMENTS --entitlements ${ARG_ENTITLEMENTS})
+  set(pass_entitlements --entitlements ${ARG_ENTITLEMENTS})
+endif()
+if(CMAKE_GENERATOR STREQUAL "Xcode")
+  # Avoid double-signing error: Since output overwrites input, Xcode runs
+  # the post-build rule even if the actual build-step was skipped.
+  set(pass_force --force)
 endif()
 
 add_custom_command(
-  TARGET ${ARG_TARGET} POST_BUILD
+  TARGET ${name} POST_BUILD
   COMMAND ${CMAKE_COMMAND} -E
   env CODESIGN_ALLOCATE=${CMAKE_CODESIGN_ALLOCATE}
   ${CMAKE_CODESIGN} -s ${LLVM_CODESIGNING_IDENTITY}
-  ${PASS_ENTITLEMENTS} $
+  ${pass_entitlements} ${pass_force} $
 )
   endif()
 endfunction()
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55328: [CMake] Revised LLDB.framework builds

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.
Herald added a subscriber: michaelplatings.



Comment at: cmake/modules/LLDBConfig.cmake:275
-if (CMAKE_SOURCE_DIR STREQUAL CMAKE_BINARY_DIR)
-  message(FATAL_ERROR "In-source builds are not allowed. CMake would overwrite 
"
-"the makefiles distributed with LLDB. Please create a directory and run cmake "

We should keep this warning, but just say that in-tree builds are not 
supported. There is no bot testing this and we don't want to bother with 
keeping this config work.


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

https://reviews.llvm.org/D55328



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

[Lldb-commits] [PATCH] D55631: Delay replacing the process till after we've finalized it

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Yeah, I'll simplify the logic after this change.

I've had mysterious crashes that I've never been able to reproduce for quite 
some time now.  Things like we go to tear down a process, the thread is 
deleting its thread plans, a thread plan goes to remove one of it's breakpoints 
and crashes because Thread::GetTarget returns a bogus target.  Then a little 
while ago I got one test in the test suite to crash this way (on my machine for 
about a day and only when I debugged it lightly...)  I figured out that the 
problem was that the Thread relies on the ProcessWP -> ProcessSP to get the 
target.  I am pretty sure this is an infrequent and racy crash because 
generally somebody else is holding onto a shared pointer to the process, so 
m_process_sp is not the last reference.  But if it is, the ThreadPlans can no 
longer get to the target.

I thought a bit about having the thread plans handle GetTarget being fallible, 
but they really do have to have a way to get to the Target when being deleted 
or they will leave stray random breakpoints in the target that will be inserted 
on rerun.  That would be no good.

This change patches over the problem for now.  It will make sure that Finalize 
gets called before destruction.

But longer term we either need to require Finalize before destruction, or we 
need to make a more robust way for Threads to get back to their Targets.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55631



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

[Lldb-commits] [PATCH] D55328: [CMake] Revised LLDB.framework builds

[Adrian Prantl via Phabricator via lldb-commits]

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

Let's give this a try.


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

https://reviews.llvm.org/D55328



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

[Lldb-commits] [PATCH] D55328: [CMake] Revised LLDB.framework builds

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Please be sure to watch all LLDB bots (green dragon and lab.llvm.org:8011) 
closely when landing this change.


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

https://reviews.llvm.org/D55328



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

[Lldb-commits] [PATCH] D55328: [CMake] Revised LLDB.framework builds

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.

Sorry for the late review, I had a look originally but didn't have any 
comments. LGTM!


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

https://reviews.llvm.org/D55328



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

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Pavel Labath via lldb-commits]

On 13/12/2018 19:32, Leonard Mosescu wrote:

What's the consensus?

Personally I think that, even considering the potential issue that Paval 
pointed out, the "target symbols add ..." is the most conservative 
approach in terms of introducing new behavior. I'm fine with the current 
directory lookup as well (the original change) since it's consistent 
with DWARF lookup.



Yes, but it also regresses existing functionality. Now if I do something 
completely nonsensical like:

(lldb) target create "/bin/ls"
Current executable set to '/bin/ls' (x86_64).
(lldb) target symbols add  -s /bin/ls /tmp/a.txt
error: symbol file '/tmp/a.txt' does not match any existing module

lldb will print a nice error for me. If I remove the safeguards like you 
did in your patch, it turns into this:

(lldb) target create "/bin/ls"
Current executable set to '/bin/ls' (x86_64).
(lldb) target symbols add  -s /bin/ls /tmp/a.txt
symbol file '/tmp/a.txt' has been added to '/bin/ls'

which is a blatant lie, because /bin/ls will continue to use symbols 
from the object file.

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

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via lldb-commits]

I think we can fix that by changing the line to:

```
if (!object_file || object_file->GetFileSpec() == symbol_fspec) {
}
```

On Thu, Dec 13, 2018 at 12:04 PM Pavel Labath  wrote:

> On 13/12/2018 19:32, Leonard Mosescu wrote:
> > What's the consensus?
> >
> > Personally I think that, even considering the potential issue that Paval
> > pointed out, the "target symbols add ..." is the most conservative
> > approach in terms of introducing new behavior. I'm fine with the current
> > directory lookup as well (the original change) since it's consistent
> > with DWARF lookup.
>
>
> Yes, but it also regresses existing functionality. Now if I do something
> completely nonsensical like:
> (lldb) target create "/bin/ls"
> Current executable set to '/bin/ls' (x86_64).
> (lldb) target symbols add  -s /bin/ls /tmp/a.txt
> error: symbol file '/tmp/a.txt' does not match any existing module
>
> lldb will print a nice error for me. If I remove the safeguards like you
> did in your patch, it turns into this:
> (lldb) target create "/bin/ls"
> Current executable set to '/bin/ls' (x86_64).
> (lldb) target symbols add  -s /bin/ls /tmp/a.txt
> symbol file '/tmp/a.txt' has been added to '/bin/ls'
>
> which is a blatant lie, because /bin/ls will continue to use symbols
> from the object file.
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via lldb-commits]

>
> I think we can fix that by changing the line to:
>
> if (!object_file || object_file->GetFileSpec() == symbol_fspec) {
> }
>

The problem is that SymbolFileNativePDB returns the module object file as
it's own object file. Are you suggesting we should track the module object
file separate from SymbolFile::m_obj_file? In a way that would be a more
accurate representation of what's going on (we don't have an ObjectFilePDB
yet) - but the check is still a bit nonsensical (using the lack of an
object file to indicate that the operation succeeded)

I agree that the case that Pavel pointed out is an issue, although the
non-nonsensical response only happens in the unlikely case you already have
symbols (so there's little reason to explicitly re-add symbols). It's also
not happening with PDBs (which is why it seemed safe).

At this point I have to ask: what is the problem with the current directory
lookup again? I seems to me that we complicated ourselves for no good
reason (using a questionable functionality in a high level Windows IDE as
reference).


On Thu, Dec 13, 2018 at 12:23 PM Zachary Turner  wrote:

> I think we can fix that by changing the line to:
>
> ```
> if (!object_file || object_file->GetFileSpec() == symbol_fspec) {
> }
> ```
>
> On Thu, Dec 13, 2018 at 12:04 PM Pavel Labath  wrote:
>
>> On 13/12/2018 19:32, Leonard Mosescu wrote:
>> > What's the consensus?
>> >
>> > Personally I think that, even considering the potential issue that
>> Paval
>> > pointed out, the "target symbols add ..." is the most conservative
>> > approach in terms of introducing new behavior. I'm fine with the
>> current
>> > directory lookup as well (the original change) since it's consistent
>> > with DWARF lookup.
>>
>>
>> Yes, but it also regresses existing functionality. Now if I do something
>> completely nonsensical like:
>> (lldb) target create "/bin/ls"
>> Current executable set to '/bin/ls' (x86_64).
>> (lldb) target symbols add  -s /bin/ls /tmp/a.txt
>> error: symbol file '/tmp/a.txt' does not match any existing module
>>
>> lldb will print a nice error for me. If I remove the safeguards like you
>> did in your patch, it turns into this:
>> (lldb) target create "/bin/ls"
>> Current executable set to '/bin/ls' (x86_64).
>> (lldb) target symbols add  -s /bin/ls /tmp/a.txt
>> symbol file '/tmp/a.txt' has been added to '/bin/ls'
>>
>> which is a blatant lie, because /bin/ls will continue to use symbols
>> from the object file.
>>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via lldb-commits]

The problems I have with current directory lookup are:

* It makes the behavior dependent on the environment, much like using an
environment variable.  This is a potential source of flakiness in tests, or
different behavior on different peoples' machines.
* It doesn't match WinDbg or MSVC
* It's temporary functionality, and temporary functionality more often than
not ends up not being so temporary, thereby contributing to technical debt.
* We already know what the permanent solution is, and we're going to have
to implement it anyway, so we could avoid this by just implementing the
permanent solution first.

Anyway, I don't want to hold this up any longer, but in the future let's
just implement the permanent solutions first so that we can avoid this type
of unnecessary blockage.

On Thu, Dec 13, 2018 at 1:10 PM Leonard Mosescu  wrote:

> I think we can fix that by changing the line to:
>>
>> if (!object_file || object_file->GetFileSpec() == symbol_fspec) {
>> }
>>
>
> The problem is that SymbolFileNativePDB returns the module object file as
> it's own object file. Are you suggesting we should track the module object
> file separate from SymbolFile::m_obj_file? In a way that would be a more
> accurate representation of what's going on (we don't have an ObjectFilePDB
> yet) - but the check is still a bit nonsensical (using the lack of an
> object file to indicate that the operation succeeded)
>
> I agree that the case that Pavel pointed out is an issue, although the
> non-nonsensical response only happens in the unlikely case you already have
> symbols (so there's little reason to explicitly re-add symbols). It's also
> not happening with PDBs (which is why it seemed safe).
>
> At this point I have to ask: what is the problem with the current
> directory lookup again? I seems to me that we complicated ourselves for no
> good reason (using a questionable functionality in a high level Windows IDE
> as reference).
>
>
> On Thu, Dec 13, 2018 at 12:23 PM Zachary Turner 
> wrote:
>
>> I think we can fix that by changing the line to:
>>
>> ```
>> if (!object_file || object_file->GetFileSpec() == symbol_fspec) {
>> }
>> ```
>>
>> On Thu, Dec 13, 2018 at 12:04 PM Pavel Labath  wrote:
>>
>>> On 13/12/2018 19:32, Leonard Mosescu wrote:
>>> > What's the consensus?
>>> >
>>> > Personally I think that, even considering the potential issue that
>>> Paval
>>> > pointed out, the "target symbols add ..." is the most conservative
>>> > approach in terms of introducing new behavior. I'm fine with the
>>> current
>>> > directory lookup as well (the original change) since it's consistent
>>> > with DWARF lookup.
>>>
>>>
>>> Yes, but it also regresses existing functionality. Now if I do something
>>> completely nonsensical like:
>>> (lldb) target create "/bin/ls"
>>> Current executable set to '/bin/ls' (x86_64).
>>> (lldb) target symbols add  -s /bin/ls /tmp/a.txt
>>> error: symbol file '/tmp/a.txt' does not match any existing module
>>>
>>> lldb will print a nice error for me. If I remove the safeguards like you
>>> did in your patch, it turns into this:
>>> (lldb) target create "/bin/ls"
>>> Current executable set to '/bin/ls' (x86_64).
>>> (lldb) target symbols add  -s /bin/ls /tmp/a.txt
>>> symbol file '/tmp/a.txt' has been added to '/bin/ls'
>>>
>>> which is a blatant lie, because /bin/ls will continue to use symbols
>>> from the object file.
>>>
>>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Greg Clayton via Phabricator via lldb-commits]

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

Just need a way to verify symbols are good. See my inline comment.




Comment at: source/Commands/CommandObjectTarget.cpp:4246
   if (symbol_file) {
-ObjectFile *object_file = symbol_file->GetObjectFile();
 

labath wrote:
> lemo wrote:
> > note I had to bypass this check: we don't (yet) have a ObjectFilePDB so the 
> > SymbolFileNativePDB always points to the associated PE binary. 
> > 
> > the check itself seems valuable as a diagnostic but not strictly required. 
> > Should I add a TODO comment and/or open a bug to revisit this?
> I not sure this is a good idea. Isn't this the only way of providing feedback 
> about whether the symbols were actually added? If we are unable to load the 
> symbol file specified (perhaps because the user made a typo, or the file is 
> corrupted), then the symbol vendor will just create a default SymbolFile 
> backed by the original object file. Doesn't that mean this will basically 
> always return true now?
> 
> I think this is strictly worse that the previous solution as it lets the 
> objectless-symbol-file hack leak out of SymbolFilePDB.
We need to add some sanity check where we ask the symbol file if it is valid. 
It should be virtual function in SymbolFile that defaults to:

```
virtual bool SymbolFile::IsValid() const {
  return GetObjectFile() != nullptr;
}
```
And we can override for SymbolFile subclasses that arenb't objfile based. How 
does this sound?


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

https://reviews.llvm.org/D55142



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

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via lldb-commits]

At this point it seems like perpetuating the hack, or at least even if
that's the direction we decide to go longterm, not implementing that
solution fully and missing some of the corner cases.

So I think I'd rather just go with the original hack of checking the
current directory at this point.  Still though, I want to make sure that
we're on the same page that in the future if we're going to need more hacks
of some kind to delay implementing a proper solution, that we shelve the
work until the proper solution is implemented and tested.  It may not be
the best thing to do in the short term, but it is in the long term, which
is what i'm trying to optimize for.

On Thu, Dec 13, 2018 at 1:39 PM Greg Clayton via Phabricator <
revi...@reviews.llvm.org> wrote:

> clayborg requested changes to this revision.
> clayborg added a comment.
> This revision now requires changes to proceed.
>
> Just need a way to verify symbols are good. See my inline comment.
>
>
>
> 
> Comment at: source/Commands/CommandObjectTarget.cpp:4246
>if (symbol_file) {
> -ObjectFile *object_file = symbol_file->GetObjectFile();
>
> 
> labath wrote:
> > lemo wrote:
> > > note I had to bypass this check: we don't (yet) have a ObjectFilePDB
> so the SymbolFileNativePDB always points to the associated PE binary.
> > >
> > > the check itself seems valuable as a diagnostic but not strictly
> required. Should I add a TODO comment and/or open a bug to revisit this?
> > I not sure this is a good idea. Isn't this the only way of providing
> feedback about whether the symbols were actually added? If we are unable to
> load the symbol file specified (perhaps because the user made a typo, or
> the file is corrupted), then the symbol vendor will just create a default
> SymbolFile backed by the original object file. Doesn't that mean this will
> basically always return true now?
> >
> > I think this is strictly worse that the previous solution as it lets the
> objectless-symbol-file hack leak out of SymbolFilePDB.
> We need to add some sanity check where we ask the symbol file if it is
> valid. It should be virtual function in SymbolFile that defaults to:
>
> ```
> virtual bool SymbolFile::IsValid() const {
>   return GetObjectFile() != nullptr;
> }
> ```
> And we can override for SymbolFile subclasses that arenb't objfile based.
> How does this sound?
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D55142/new/
>
> https://reviews.llvm.org/D55142
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55571: [ast] CreateParameterDeclaration should use an appropriate DeclContext.

[Frederic Riss via Phabricator via lldb-commits]

friss added a comment.

Zachary, how did you figure out this can be an issue? Does it fix something we 
should be testing?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55571



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

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via lldb-commits]

Thanks Zach. Don't get me wrong, I have no problem tweaking it as long as
necessary assuming we all agree on the plan: we could implement a
ObjectFilePDB and a PDB SymbolVendor first, then the contentious PDB lookup
detail goes away.

My intention was to enable other developers to start consuming PDBs with
minidumps, but perhaps I rushed ahead too much. The long discussion clearly
suggests that the changes are not ready, so I'll shelve them for now and
keep them until the timing is right.


> * It makes the behavior dependent on the environment, much like using an
> environment variable.  This is a potential source of flakiness in tests, or
> different behavior on different peoples' machines.
>
I mostly agree. Although 1) this matches what LLDB already does for DWARF
and 2) I think the issues with flakiness are a bit overblown: there's a lot
of stuff which depends on the CWD, and the environment for tests should be
predictable in general.


> * It doesn't match WinDbg or MSVC
>
Sure, but neither does looking next to the .dmp file (which is purely a
VisualStudio invention - and any IDE can do the same on top of LLDB if they
really choose to, but it should not be backed in IMO).


> * It's temporary functionality, and temporary functionality more often
> than not ends up not being so temporary, thereby contributing to technical
> debt.
>
If we unify the logic with the DWARF SymbolVendor then only the
implementation itself it temporary, the lookup logic would not change,
right?


> * We already know what the permanent solution is, and we're going to have
> to implement it anyway, so we could avoid this by just implementing the
> permanent solution first
>
The catch22 is that we can't test anything else involving minidumps + PDBs
in the meantime. I found that exercising that combination to be very useful
in uncovering other parts which need attention.

Also, just a sanity check: what do you think is the permanent solution?


On Thu, Dec 13, 2018 at 1:44 PM Zachary Turner  wrote:

> At this point it seems like perpetuating the hack, or at least even if
> that's the direction we decide to go longterm, not implementing that
> solution fully and missing some of the corner cases.
>
> So I think I'd rather just go with the original hack of checking the
> current directory at this point.  Still though, I want to make sure that
> we're on the same page that in the future if we're going to need more hacks
> of some kind to delay implementing a proper solution, that we shelve the
> work until the proper solution is implemented and tested.  It may not be
> the best thing to do in the short term, but it is in the long term, which
> is what i'm trying to optimize for.
>
> On Thu, Dec 13, 2018 at 1:39 PM Greg Clayton via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> clayborg requested changes to this revision.
>> clayborg added a comment.
>> This revision now requires changes to proceed.
>>
>> Just need a way to verify symbols are good. See my inline comment.
>>
>>
>>
>> 
>> Comment at: source/Commands/CommandObjectTarget.cpp:4246
>>if (symbol_file) {
>> -ObjectFile *object_file = symbol_file->GetObjectFile();
>>
>> 
>> labath wrote:
>> > lemo wrote:
>> > > note I had to bypass this check: we don't (yet) have a ObjectFilePDB
>> so the SymbolFileNativePDB always points to the associated PE binary.
>> > >
>> > > the check itself seems valuable as a diagnostic but not strictly
>> required. Should I add a TODO comment and/or open a bug to revisit this?
>> > I not sure this is a good idea. Isn't this the only way of providing
>> feedback about whether the symbols were actually added? If we are unable to
>> load the symbol file specified (perhaps because the user made a typo, or
>> the file is corrupted), then the symbol vendor will just create a default
>> SymbolFile backed by the original object file. Doesn't that mean this will
>> basically always return true now?
>> >
>> > I think this is strictly worse that the previous solution as it lets
>> the objectless-symbol-file hack leak out of SymbolFilePDB.
>> We need to add some sanity check where we ask the symbol file if it is
>> valid. It should be virtual function in SymbolFile that defaults to:
>>
>> ```
>> virtual bool SymbolFile::IsValid() const {
>>   return GetObjectFile() != nullptr;
>> }
>> ```
>> And we can override for SymbolFile subclasses that arenb't objfile based.
>> How does this sound?
>>
>>
>> CHANGES SINCE LAST ACTION
>>   https://reviews.llvm.org/D55142/new/
>>
>> https://reviews.llvm.org/D55142
>>
>>
>>
>>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via lldb-commits]

The permanent solution would be figuring out what to do about the
ObjectFile situation (e.g. do we want to make an ObjectFilePDB or do we
want to live in a world where SymbolFiles need not be backed by
ObjectFiles?), and then regardless of what we decide there, implementing
the SymbolVendor that can search a combination of locations including (but
not limited to) a symbol server.  lldb already has the notion of a symbol
path.  So using this we could just use that existing logic to search the
symbol path, and before you run LLDB make sure your minidump .pdb files are
in that search path.  If we make an ObjectFilePDB, then this should be able
to fall out naturally of the existing design with no additional work,
because it will use the normal SymbolVendor search logic.  If we allow
SymbolFilePDB to live without an ObjectFilePDB, then we would have to make
changes similar to what you proposed earlier where we can still get a
SymbolVendor and use it to execute its default search algorithm, plus
probably some other changes to make sure various other things work
correctly in the presence of ObjectFile-less SymbolFiles.

On Thu, Dec 13, 2018 at 1:59 PM Leonard Mosescu  wrote:

> Thanks Zach. Don't get me wrong, I have no problem tweaking it as long as
> necessary assuming we all agree on the plan: we could implement a
> ObjectFilePDB and a PDB SymbolVendor first, then the contentious PDB lookup
> detail goes away.
>
> My intention was to enable other developers to start consuming PDBs with
> minidumps, but perhaps I rushed ahead too much. The long discussion clearly
> suggests that the changes are not ready, so I'll shelve them for now and
> keep them until the timing is right.
>
>
>> * It makes the behavior dependent on the environment, much like using an
>> environment variable.  This is a potential source of flakiness in tests, or
>> different behavior on different peoples' machines.
>>
> I mostly agree. Although 1) this matches what LLDB already does for DWARF
> and 2) I think the issues with flakiness are a bit overblown: there's a lot
> of stuff which depends on the CWD, and the environment for tests should be
> predictable in general.
>
>
>> * It doesn't match WinDbg or MSVC
>>
> Sure, but neither does looking next to the .dmp file (which is purely a
> VisualStudio invention - and any IDE can do the same on top of LLDB if they
> really choose to, but it should not be backed in IMO).
>
>
>> * It's temporary functionality, and temporary functionality more often
>> than not ends up not being so temporary, thereby contributing to technical
>> debt.
>>
> If we unify the logic with the DWARF SymbolVendor then only the
> implementation itself it temporary, the lookup logic would not change,
> right?
>
>
>> * We already know what the permanent solution is, and we're going to have
>> to implement it anyway, so we could avoid this by just implementing the
>> permanent solution first
>>
> The catch22 is that we can't test anything else involving minidumps + PDBs
> in the meantime. I found that exercising that combination to be very useful
> in uncovering other parts which need attention.
>
> Also, just a sanity check: what do you think is the permanent solution?
>
>
> On Thu, Dec 13, 2018 at 1:44 PM Zachary Turner  wrote:
>
>> At this point it seems like perpetuating the hack, or at least even if
>> that's the direction we decide to go longterm, not implementing that
>> solution fully and missing some of the corner cases.
>>
>> So I think I'd rather just go with the original hack of checking the
>> current directory at this point.  Still though, I want to make sure that
>> we're on the same page that in the future if we're going to need more hacks
>> of some kind to delay implementing a proper solution, that we shelve the
>> work until the proper solution is implemented and tested.  It may not be
>> the best thing to do in the short term, but it is in the long term, which
>> is what i'm trying to optimize for.
>>
>> On Thu, Dec 13, 2018 at 1:39 PM Greg Clayton via Phabricator <
>> revi...@reviews.llvm.org> wrote:
>>
>>> clayborg requested changes to this revision.
>>> clayborg added a comment.
>>> This revision now requires changes to proceed.
>>>
>>> Just need a way to verify symbols are good. See my inline comment.
>>>
>>>
>>>
>>> 
>>> Comment at: source/Commands/CommandObjectTarget.cpp:4246
>>>if (symbol_file) {
>>> -ObjectFile *object_file = symbol_file->GetObjectFile();
>>>
>>> 
>>> labath wrote:
>>> > lemo wrote:
>>> > > note I had to bypass this check: we don't (yet) have a ObjectFilePDB
>>> so the SymbolFileNativePDB always points to the associated PE binary.
>>> > >
>>> > > the check itself seems valuable as a diagnostic but not strictly
>>> required. Should I add a TODO comment and/or open a bug to revisit this?
>>> > I not sure this is a good idea. Isn't this the only way of providing
>>> feedback about whether the symbols were actually added? If we ar

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via lldb-commits]

That said, as you mentioned this enables other developers to start working
on things, and if that means we can get the SymbolVendor in more quickly,
then we can get rid of this more quickly.  I just really want to converge
towards the permanent solution, rather than away from it, so if committing
this gets us there quicker then I can get behind it.  I trust your
judgement here :)

On Thu, Dec 13, 2018 at 2:19 PM Zachary Turner  wrote:

> The permanent solution would be figuring out what to do about the
> ObjectFile situation (e.g. do we want to make an ObjectFilePDB or do we
> want to live in a world where SymbolFiles need not be backed by
> ObjectFiles?), and then regardless of what we decide there, implementing
> the SymbolVendor that can search a combination of locations including (but
> not limited to) a symbol server.  lldb already has the notion of a symbol
> path.  So using this we could just use that existing logic to search the
> symbol path, and before you run LLDB make sure your minidump .pdb files are
> in that search path.  If we make an ObjectFilePDB, then this should be able
> to fall out naturally of the existing design with no additional work,
> because it will use the normal SymbolVendor search logic.  If we allow
> SymbolFilePDB to live without an ObjectFilePDB, then we would have to make
> changes similar to what you proposed earlier where we can still get a
> SymbolVendor and use it to execute its default search algorithm, plus
> probably some other changes to make sure various other things work
> correctly in the presence of ObjectFile-less SymbolFiles.
>
> On Thu, Dec 13, 2018 at 1:59 PM Leonard Mosescu 
> wrote:
>
>> Thanks Zach. Don't get me wrong, I have no problem tweaking it as long as
>> necessary assuming we all agree on the plan: we could implement a
>> ObjectFilePDB and a PDB SymbolVendor first, then the contentious PDB lookup
>> detail goes away.
>>
>> My intention was to enable other developers to start consuming PDBs with
>> minidumps, but perhaps I rushed ahead too much. The long discussion clearly
>> suggests that the changes are not ready, so I'll shelve them for now and
>> keep them until the timing is right.
>>
>>
>>> * It makes the behavior dependent on the environment, much like using an
>>> environment variable.  This is a potential source of flakiness in tests, or
>>> different behavior on different peoples' machines.
>>>
>> I mostly agree. Although 1) this matches what LLDB already does for DWARF
>> and 2) I think the issues with flakiness are a bit overblown: there's a lot
>> of stuff which depends on the CWD, and the environment for tests should be
>> predictable in general.
>>
>>
>>> * It doesn't match WinDbg or MSVC
>>>
>> Sure, but neither does looking next to the .dmp file (which is purely a
>> VisualStudio invention - and any IDE can do the same on top of LLDB if they
>> really choose to, but it should not be backed in IMO).
>>
>>
>>> * It's temporary functionality, and temporary functionality more often
>>> than not ends up not being so temporary, thereby contributing to technical
>>> debt.
>>>
>> If we unify the logic with the DWARF SymbolVendor then only the
>> implementation itself it temporary, the lookup logic would not change,
>> right?
>>
>>
>>> * We already know what the permanent solution is, and we're going to
>>> have to implement it anyway, so we could avoid this by just implementing
>>> the permanent solution first
>>>
>> The catch22 is that we can't test anything else involving minidumps +
>> PDBs in the meantime. I found that exercising that combination to be very
>> useful in uncovering other parts which need attention.
>>
>> Also, just a sanity check: what do you think is the permanent solution?
>>
>>
>> On Thu, Dec 13, 2018 at 1:44 PM Zachary Turner 
>> wrote:
>>
>>> At this point it seems like perpetuating the hack, or at least even if
>>> that's the direction we decide to go longterm, not implementing that
>>> solution fully and missing some of the corner cases.
>>>
>>> So I think I'd rather just go with the original hack of checking the
>>> current directory at this point.  Still though, I want to make sure that
>>> we're on the same page that in the future if we're going to need more hacks
>>> of some kind to delay implementing a proper solution, that we shelve the
>>> work until the proper solution is implemented and tested.  It may not be
>>> the best thing to do in the short term, but it is in the long term, which
>>> is what i'm trying to optimize for.
>>>
>>> On Thu, Dec 13, 2018 at 1:39 PM Greg Clayton via Phabricator <
>>> revi...@reviews.llvm.org> wrote:
>>>
 clayborg requested changes to this revision.
 clayborg added a comment.
 This revision now requires changes to proceed.

 Just need a way to verify symbols are good. See my inline comment.



 
 Comment at: source/Commands/CommandObjectTarget.cpp:4246
if (symbol_file) {
 -ObjectFile 

[Lldb-commits] [PATCH] D55571: [ast] CreateParameterDeclaration should use an appropriate DeclContext.

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

In D55571#1330354 , @friss wrote:

> Zachary, how did you figure out this can be an issue? Does it fix something 
> we should be testing?


Recently I added a `target modules dump ast` command and I've been using it to 
write tests against (see lldb/lit/SymbolFile/NativePDB for an example of what 
some of these tests look like).  While experimenting with this command, I 
noticed that I had some ParmVarDecls under the translation unit decl (e.g. 
global scope), which obviously is something that isn't possible.  I only tested 
this with the PDB parser, so I can't confirm whether the same situation could 
occur int he DWARF parser, but since this is strictly building the AST *after* 
we've parsed the debug info, I definitely think it would be a problem for DWARF.

So the million dollar question is: What happens when you have function 
parameters in the AST at global scope?  I think it falls into "undefined 
behavior" territory - i.e. there's no way to know what clang will do, since 
you're not supposed to have that situation.

As for as testing, I've found this command extremely useful for writing ast 
reconstruction tests -- Do a few things in the debugger, run the command, 
FileCheck the output.  I think "did we construct a valid AST from the debug 
info?" is a largely unexplored testing surface in LLDB, so this definitely 
opens up a lot of possibilities for testing on this front.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55571



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

[Lldb-commits] [PATCH] D55116: [CMake] llvm_codesign workaround for Xcode double-signing errors

[Chris Bieneman via Phabricator via lldb-commits]

beanz added a comment.

Sorry for being behind on this, but I don't think this is the right solution to 
the problem.

I don't think we should ever pass `--force`. To avoid duplicate code signing 
when using the Xcode generator we should set the 
`XCODE_ATTRIBUTE_CODE_SIGN_IDENTITY` target property so that Xcode actually 
knows what signing identity to use and can properly build the target.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55116



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

[Lldb-commits] [PATCH] D55582: [Reproducers] Add command reproducer

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

I played around with this today:

- I'm totally convinced that we should replay by swapping out stdin with a file 
from the reproducer. This makes this better overall.
- I'm not convinced that capturing just stdin is an improvement. What I like 
about the current approach is that we don't have to bother with the `-o`/`-S` 
commands when replaying. We already have a mechanism to deal with them so I 
don't think it's unreasonable to piggyback on it, especially because this is 
how we display it to the user. On the other hand, I generally try to keep the 
code path the same between the regular and reproducer case, and processing the 
command line options in the reproducer case would reuse more of the existing 
code. Finally, we'd have to figure out a way to test this, because currently we 
rely on the commands provided/sourced through the command line to test the 
replay. How would we test this functionality when only capturing from stdin?


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

https://reviews.llvm.org/D55582



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

[Lldb-commits] [lldb] r349122 - Fix test failures that depended on module order

[Greg Clayton via lldb-commits]

Author: gclayton
Date: Thu Dec 13 19:07:15 2018
New Revision: 349122

URL: http://llvm.org/viewvc/llvm-project?rev=349122&view=rev
Log:
Fix test failures that depended on module order

Modified:

lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py

lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py?rev=349122&r1=349121&r2=349122&view=diff
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
 (original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
 Thu Dec 13 19:07:15 2018
@@ -97,14 +97,18 @@ class MiniDumpNewTestCase(TestBase):
 self.assertTrue(self.process, PROCESS_IS_VALID)
 expected_modules = [
 {
-'filename' : 'linux-gate.so',
-'uuid' : '4EAD28F8-88EF-3520-872B-73C6F2FE7306-C41AF22F',
+'filename' : 'linux-x86_64',
+'uuid' : 'E35C283B-C327-C287-62DB-788BF5A4078B-E2351448',
 },
 {
 'filename' : 'libm-2.19.so',
 'uuid' : 'D144258E-6149-00B2-55A3-1F3FD2283A87-8670D5BC',
 },
 {
+'filename' : 'libgcc_s.so.1',
+'uuid' : '36311B44-5771-0AE5-578C-4BF00791DED7-359DBB92',
+},
+{
 'filename' : 'libstdc++.so.6.0.19',
 'uuid' : '76190E92-2AF7-457D-078F-75C9B15FA184-E83EB506',
 },
@@ -113,24 +117,20 @@ class MiniDumpNewTestCase(TestBase):
 'uuid' : 'CF699A15-CAAE-64F5-0311-FC4655B86DC3-9A479789',
 },
 {
-'filename' : 'linux-x86_64',
-'uuid' : 'E35C283B-C327-C287-62DB-788BF5A4078B-E2351448',
-},
-{
-'filename' : 'libgcc_s.so.1',
-'uuid' : '36311B44-5771-0AE5-578C-4BF00791DED7-359DBB92',
-},
-{
 'filename' : 'libpthread-2.19.so',
 'uuid' : '31E9F21A-E8C1-0396-171F-1E13DA157809-86FA696C',
 },
 {
+'filename' : 'libbreakpad.so',
+'uuid' : '784FD549-332D-826E-D23F-18C17C6F320A',
+},
+{
 'filename' : 'ld-2.19.so',
 'uuid' : 'D0F53790-4076-D73F-29E4-A37341F8A449-E2EF6CD0',
 },
 {
-'filename' : 'libbreakpad.so',
-'uuid' : '784FD549-332D-826E-D23F-18C17C6F320A',
+'filename' : 'linux-gate.so',
+'uuid' : '4EAD28F8-88EF-3520-872B-73C6F2FE7306-C41AF22F',
 },
 ]
 self.assertEqual(self.target.GetNumModules(), len(expected_modules))

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py?rev=349122&r1=349121&r2=349122&view=diff
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
 (original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
 Thu Dec 13 19:07:15 2018
@@ -50,28 +50,28 @@ class MiniDumpTestCase(TestBase):
 self.assertTrue(self.process, PROCESS_IS_VALID)
 expected_modules = [
 {
-'filename' : r"C:\Windows\System32/MSVCP120D.dll",
-'uuid' : '6E51053C-E757-EB40-8D3F-9722C5BD80DD-0100',
-},
-{
-'filename' : r"C:\Windows\SysWOW64/kernel32.dll",
-'uuid' : '1B7ECBE5-5E00-1341-AB98-98D6913B52D8-0200',
-},
-{
 'filename' : r"C:\Users\amccarth\Documents\Visual Studio 
2013\Projects\fizzbuzz\Debug/fizzbuzz.exe",
 'uuid' : '91B7450F-969A-F946-BF8F-2D6076EA421A-1100',
 },
 {
-'filename' : r"C:\Windows\System32/MSVCR120D.dll",
-'uuid' : '86FB8263-C446-4640-AE42-8D97B3F91FF2-0100',
+'filename' : r"C:\Windows\SysWOW64/ntdll.dll",
+'uuid' : '6A84B0BB-2C40-5240-A16B-67650BBFE6B0-0200',
+},
+{
+'filename' : r"C:\Windows\SysWOW64/kernel32.dll",
+'uuid' : '1B7ECBE5-5E00-1341-AB98-98D6913B52D8-0200',
 },
 {
   

[Lldb-commits] [PATCH] D55522: Cache memory regions in ProcessMinidump and use the linux maps as the source of the information if available.

[Greg Clayton via Phabricator via lldb-commits]

clayborg updated this revision to Diff 178168.
clayborg added a comment.
Herald added a subscriber: mgorny.

Fixed errors, fully test, and make static function that are local to 
MinidumpParser.cpp that parse the region info from linux maps, memory info 
list, memory list and memory 64 list.


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

https://reviews.llvm.org/D55522

Files:
  include/lldb/Target/MemoryRegionInfo.h
  lldb.xcodeproj/project.pbxproj
  source/Plugins/Process/Utility/CMakeLists.txt
  source/Plugins/Process/Utility/LinuxProcMaps.cpp
  source/Plugins/Process/Utility/LinuxProcMaps.h
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/MinidumpTypes.h
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  unittests/Process/minidump/Inputs/regions-linux-map.dmp
  unittests/Process/minidump/Inputs/regions-memlist.dmp
  unittests/Process/minidump/Inputs/regions-memlist64.dmp
  unittests/Process/minidump/MinidumpParserTest.cpp

Index: unittests/Process/minidump/MinidumpParserTest.cpp
===
--- unittests/Process/minidump/MinidumpParserTest.cpp
+++ unittests/Process/minidump/MinidumpParserTest.cpp
@@ -300,31 +300,122 @@
   EXPECT_FALSE(parser->FindMemoryRange(0x7ffe + 4096).hasValue());
 }
 
-void check_region_info(std::unique_ptr &parser,
-   const uint64_t addr, MemoryRegionInfo::OptionalBool read,
-   MemoryRegionInfo::OptionalBool write,
-   MemoryRegionInfo::OptionalBool exec) {
+void check_region(std::unique_ptr &parser,
+  lldb::addr_t addr, lldb::addr_t start, lldb::addr_t end,
+  MemoryRegionInfo::OptionalBool read,
+  MemoryRegionInfo::OptionalBool write,
+  MemoryRegionInfo::OptionalBool exec,
+  MemoryRegionInfo::OptionalBool mapped,
+  ConstString name = ConstString()) {
   auto range_info = parser->GetMemoryRegionInfo(addr);
-  ASSERT_TRUE(range_info.hasValue());
-  EXPECT_EQ(read, range_info->GetReadable());
-  EXPECT_EQ(write, range_info->GetWritable());
-  EXPECT_EQ(exec, range_info->GetExecutable());
+  EXPECT_EQ(start, range_info.GetRange().GetRangeBase());
+  EXPECT_EQ(end, range_info.GetRange().GetRangeEnd());
+  EXPECT_EQ(read, range_info.GetReadable());
+  EXPECT_EQ(write, range_info.GetWritable());
+  EXPECT_EQ(exec, range_info.GetExecutable());
+  EXPECT_EQ(mapped, range_info.GetMapped());
+  EXPECT_EQ(name, range_info.GetName());
 }
 
+// Same as above function where addr == start
+void check_region(std::unique_ptr &parser,
+  lldb::addr_t start, lldb::addr_t end,
+  MemoryRegionInfo::OptionalBool read,
+  MemoryRegionInfo::OptionalBool write,
+  MemoryRegionInfo::OptionalBool exec,
+  MemoryRegionInfo::OptionalBool mapped,
+  ConstString name = ConstString()) {
+  check_region(parser, start, start, end, read, write, exec, mapped, name);
+}
+
+
+constexpr auto yes = MemoryRegionInfo::eYes;
+constexpr auto no = MemoryRegionInfo::eNo;
+constexpr auto unknown = MemoryRegionInfo::eDontKnow;
+
 TEST_F(MinidumpParserTest, GetMemoryRegionInfo) {
   SetUpData("fizzbuzz_wow64.dmp");
 
-  const auto yes = MemoryRegionInfo::eYes;
-  const auto no = MemoryRegionInfo::eNo;
+  check_region(parser, 0x, 0x0001, no, no, no, no);
+  check_region(parser, 0x0001, 0x0002, yes, yes, no, yes);
+  check_region(parser, 0x0002, 0x0003, yes, yes, no, yes);
+  check_region(parser, 0x0003, 0x00031000, yes, yes, no, yes);
+  check_region(parser, 0x00031000, 0x0004, no, no, no, no);
+  check_region(parser, 0x0004, 0x00041000, yes, no, no, yes);
 
-  check_region_info(parser, 0x0, no, no, no);
-  check_region_info(parser, 0x1, yes, yes, no);
-  check_region_info(parser, 0x2, yes, yes, no);
-  check_region_info(parser, 0x3, yes, yes, no);
-  check_region_info(parser, 0x31000, no, no, no);
-  check_region_info(parser, 0x4, yes, no, no);
+  // Check addresses contained inside ranges
+  check_region(parser, 0x0001, 0x, 0x0001, no, no, no, no);
+  check_region(parser, 0x, 0x, 0x0001, no, no, no, no);
+  check_region(parser, 0x00010001, 0x0001, 0x0002, yes, yes, no, yes);
+  check_region(parser, 0x0001, 0x0001, 0x0002, yes, yes, no, yes);
+
+  // Test that an address after the last entry maps to rest of the memory space
+  check_region(parser, 0x7fff, 0x7fff, UINT64_MAX, no, no, no, no);
 }
 
+TEST_F(MinidumpParserTest, GetMemoryRegionInfoFromMemoryList) {
+  SetUpData("regions-memlist.dmp");
+  // Test we can get memory regions from the MINIDUMP_MEMORY_LIST stream when
+  // we don't have a MemoryInfoListStream.
+
+  // Test addres before the first entry comes back with not

[Lldb-commits] [PATCH] D55116: [CMake] llvm_codesign workaround for Xcode double-signing errors

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz added a comment.

Fair enough. Will create a ticket for it. At least this works for now.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55116



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