[Lldb-commits] [PATCH] D55574: Remove else statements after returns

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere abandoned this revision.
JDevlieghere added a comment.

I'm going to abandon this for now until we have better tooling to address Jim's 
concerns here. I'm also not super happy with the formatting, which seems to be 
off in quite a few locations. The problem is that clang-tidy knows to reformat 
the source range it touched, but in this case the code below is affected. I 
hacked this up by having clang-format looking one line below the lines that 
were changed, but looks like that was not sufficient. Also a few case are 
covered by D55584  so definitely want to land 
that first.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55574



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

[Lldb-commits] [PATCH] D55584: [LLDB] Simplify Boolean expressions

[Eugene Zelenko via Phabricator via lldb-commits]

Eugene.Zelenko added a comment.

Thank you for cleanup effort!

I would suggest to also run modernize checks and at least next readability 
checks:

readability-container-size-empty
readability-isolate-declaration
readability-redundant-control-flow
readability-redundant-member-init
readability-redundant-smartptr-get
readability-redundant-string-cstr
readability-redundant-string-init

Indeed, Clang-tidy offers even more useful checks.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55584



___
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 Tue, Dec 11, 2018 at 4:22 PM Leonard Mosescu  wrote:

> I guess I don't see why we need a temporary solution at all.  If we can
>> have logic that can be rolled into the SymbolVendor when we get it, and
>> makes sense there, and is also simple, why not go with it?  Failing that,
>> doesn't the `target symbols add` solution also work fine?
>>
>
> I just checked again, and "target symbols add" depends on having a real
> SymbolVendor implementation. So unfortunately we still need a temporary
> solution.
>

I tried to verify this behavior, but it seems like it should already work
out of the box?   So we're on the same page, we already do have a real
SymbolVendor implementation, it just happens to be the *default*
SymbolVendor implementation.  It's not the case that one doesn't exist at
all.

So anyway, when I run "target symbols add foo.exe path/to/foo.pdb"
internally what this does is set a member variable called m_symfile_spec on
the Module object.  Then, it calls our normal CalculateAbilities() function
which calls loadMatchingPDBFile.

I think all that needs to happen is that we need to check this field.  If
it's empty, try to load a matching PDB file.  If it's set to something,
then load that file instead.
___
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 guess I don't see why we need a temporary solution at all.  If we can
> have logic that can be rolled into the SymbolVendor when we get it, and
> makes sense there, and is also simple, why not go with it?  Failing that,
> doesn't the `target symbols add` solution also work fine?
>

I just checked again, and "target symbols add" depends on having a real
SymbolVendor implementation. So unfortunately we still need a temporary
solution.

So far we're left with:
1) look in the current directory
2) look in the same directory as the .dmp file (we can't use the .exe/.dll
locations since we don't have them in this case)

I know we had a bit of back and forth about this. After revisiting the code
(and for what is worth taking Visual Studio and windbg behavior in
consideration), I still think that the current directory is the best
choice. It's consistent with what LLDB already does for DWARF, so it's not
adding another magic lookup.

On Tue, Dec 11, 2018 at 3:41 PM Adrian McCarthy  wrote:

> >  But here, we're talking about a situation where there is no EXE, only
> a minidump.  If there is a minidump and no EXE then neither WinDbg nor VS
> will search the minidump folder for the PDB.
>
> For the record, the experiments do not bear this out.  VS will indeed
> search in the minidump folder for the PDB.  Unfortunately, a lot of this
> conversation was taken offline.
>
> On Tue, Dec 11, 2018 at 3:30 PM Zachary Turner via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> zturner added a comment.
>>
>> In D55142#1326247 , @lemo wrote:
>>
>> > > How large is the PDB file here?
>> >
>> > ~100kb
>>
>>
>> We have a couple of tests in LLVM where PDB files are checked in, but
>> they are very few.  We cannot explode the repo with large numbers of binary
>> files.  So this is probably fine, but if this becomes a pattern, we will
>> need to come up with a different solution.
>>
>>
>> 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

[Adrian McCarthy via lldb-commits]

>  But here, we're talking about a situation where there is no EXE, only a
minidump.  If there is a minidump and no EXE then neither WinDbg nor VS
will search the minidump folder for the PDB.

For the record, the experiments do not bear this out.  VS will indeed
search in the minidump folder for the PDB.  Unfortunately, a lot of this
conversation was taken offline.

On Tue, Dec 11, 2018 at 3:30 PM Zachary Turner via Phabricator <
revi...@reviews.llvm.org> wrote:

> zturner added a comment.
>
> In D55142#1326247 , @lemo wrote:
>
> > > How large is the PDB file here?
> >
> > ~100kb
>
>
> We have a couple of tests in LLVM where PDB files are checked in, but they
> are very few.  We cannot explode the repo with large numbers of binary
> files.  So this is probably fine, but if this becomes a pattern, we will
> need to come up with a different solution.
>
>
> 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] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

In D55142#1326247 , @lemo wrote:

> > How large is the PDB file here?
>
> ~100kb


We have a couple of tests in LLVM where PDB files are checked in, but they are 
very few.  We cannot explode the repo with large numbers of binary files.  So 
this is probably fine, but if this becomes a pattern, we will need to come up 
with a different solution.


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]

I guess I don't see why we need a temporary solution at all.  If we can
have logic that can be rolled into the SymbolVendor when we get it, and
makes sense there, and is also simple, why not go with it?  Failing that,
doesn't the `target symbols add` solution also work fine?

On Tue, Dec 11, 2018 at 3:24 PM Leonard Mosescu  wrote:

> But here, we're talking about a situation where there is no EXE, only a
>> minidump.  If there is a minidump and no EXE then neither WinDbg nor VS
>> will search the minidump folder for the PDB.
>
>
> Indeed, this is key part.
>
>
>>  In my mind, the algorithm could be something like: ...
>>
>
> I'm not a big fan of this kind of magic. VS sometime does it, and while
> it's nice in the 80% of the cases I found it hard to troubleshoot when
> something goes wrong. For what it's worth, recent versions of VC++ debugger
> are more aligned with ntsd/cdb/windbg.
>
> Anyway, for this patch I think the question is: what's the simplest
> temporary solution? All of these ideas are interesting but more appropriate
> to evaluate when we start designing the PDB SymbolVendor.
>
>
>
> On Tue, Dec 11, 2018 at 3:05 PM Zachary Turner  wrote:
>
>> Only one way to know for sure, and that's to test it :)  So I did.
>>
>> Yes, it will search the directory of the EXE for the PDB.  But here,
>> we're talking about a situation where there is no EXE, only a minidump.  If
>> there is a minidump and no EXE then neither WinDbg nor VS will search the
>> minidump folder for the PDB.  Personally I think it should and I wouldn't
>> mind if that were a documented and tested feature of LLDB, because since it
>> already is established behavior to search the minidump folder for the EXE,
>> it doesn't seem hacky at all for it to also search that location for the
>> PDB.  In my mind, the algorithm could be something like:
>>
>> ```
>> if (ExeLocation = FindExecutableInMinidumpFolder()) {
>>   PdbLocation = FindPdbFromExecutable(ExeLocation)
>> } else {
>>   PdbLocation = FindPdbInMinidumpFolder();
>>   if (!PdbLocation)
>> PdbLocation = FindPdbInSympath();
>> }
>> ```
>>
>> and that would be pretty logical and not code that we would have to
>> consider temporary.
>>
>> My main pushback here is that it's harder to remove code than it is to
>> add it, because once you add it, someone is going to depend on it and
>> complain when you try to remove it.  So if we can just establish some
>> behavior that satisfies the use case while not being temporary, I would
>> prefer to do it.
>>
>> Another option I can think of is to just run `target symbols add -s
>> foo.exe foo.pdb`.  I think we would need to have SymbolFileNativePDB check
>> the value of this setting, but using this workflow, you could just put a
>> .lldbinit file next to lldb.exe that runs this command at startup.
>>
>> On Tue, Dec 11, 2018 at 2:17 PM Adrian McCarthy 
>> wrote:
>>
>>> I believe the PDB is searched for in the EXE directory before the symbol
>>> search path is used.  At least, that's what it used to do, back when I used
>>> VS debugger for post-mortem debugging.  It was the only sane way to ensure
>>> it would find the right version of the PDB if you didn't have a local
>>> symbol server.
>>>
>>> Yes, I understand that the security note was about DLL loading.  My
>>> point was that, in general, Windows looks in well known places first,
>>> before checking the current working directory.
>>>
>>> On Tue, Dec 11, 2018 at 2:12 PM Leonard Mosescu 
>>> wrote:
>>>
 The Windowsy thing to do is what Zach said:  Check the directory that
> contains the .dmp for the .pdb.  It's the first place the VS debugger 
> looks
> when opening a minidump.  It's less sensitive to the user's environment.
> (Making the user change the current working directory for this could be at
> odds with other bits of the software that look relative the cwd.)
>
> https://docs.microsoft.com/en-us/visualstudio/debugger/using-dump-files?view=vs-2017#BKMK_Find_binaries__symbol___pdb__files__and_source_files
>

 Except that it doesn't :) Neither VisualStudio nor the Windows
 Debuggers (windbg & co) look for PDBs in the same directory as the dump
 file. The search is controlled by an explicit "symbol search path". The
 link you mentioned is a bit confusing indeed (although it only claims that
 the .exe's are searched in the same directory as the .dmp)

 While security is not a big issue here, note that Windows generally
> searches for DLLs in the known/expected places _before_ checking to see if
> it's in the current working directory.  This prevents a sneaky download
> from effectively replacing a DLL.  Replacing a PDB is probably less of a
> concern.
>
> https://docs.microsoft.com/en-us/windows/desktop/dlls/dynamic-link-library-search-order#standard-search-order-for-desktop-applications
>

 This is about the runtime dynamic module loader, not the debugger.




 On 

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

[Leonard Mosescu via lldb-commits]

>
> But here, we're talking about a situation where there is no EXE, only a
> minidump.  If there is a minidump and no EXE then neither WinDbg nor VS
> will search the minidump folder for the PDB.


Indeed, this is key part.


>  In my mind, the algorithm could be something like: ...
>

I'm not a big fan of this kind of magic. VS sometime does it, and while
it's nice in the 80% of the cases I found it hard to troubleshoot when
something goes wrong. For what it's worth, recent versions of VC++ debugger
are more aligned with ntsd/cdb/windbg.

Anyway, for this patch I think the question is: what's the simplest
temporary solution? All of these ideas are interesting but more appropriate
to evaluate when we start designing the PDB SymbolVendor.



On Tue, Dec 11, 2018 at 3:05 PM Zachary Turner  wrote:

> Only one way to know for sure, and that's to test it :)  So I did.
>
> Yes, it will search the directory of the EXE for the PDB.  But here, we're
> talking about a situation where there is no EXE, only a minidump.  If there
> is a minidump and no EXE then neither WinDbg nor VS will search the
> minidump folder for the PDB.  Personally I think it should and I wouldn't
> mind if that were a documented and tested feature of LLDB, because since it
> already is established behavior to search the minidump folder for the EXE,
> it doesn't seem hacky at all for it to also search that location for the
> PDB.  In my mind, the algorithm could be something like:
>
> ```
> if (ExeLocation = FindExecutableInMinidumpFolder()) {
>   PdbLocation = FindPdbFromExecutable(ExeLocation)
> } else {
>   PdbLocation = FindPdbInMinidumpFolder();
>   if (!PdbLocation)
> PdbLocation = FindPdbInSympath();
> }
> ```
>
> and that would be pretty logical and not code that we would have to
> consider temporary.
>
> My main pushback here is that it's harder to remove code than it is to add
> it, because once you add it, someone is going to depend on it and complain
> when you try to remove it.  So if we can just establish some behavior that
> satisfies the use case while not being temporary, I would prefer to do it.
>
> Another option I can think of is to just run `target symbols add -s
> foo.exe foo.pdb`.  I think we would need to have SymbolFileNativePDB check
> the value of this setting, but using this workflow, you could just put a
> .lldbinit file next to lldb.exe that runs this command at startup.
>
> On Tue, Dec 11, 2018 at 2:17 PM Adrian McCarthy 
> wrote:
>
>> I believe the PDB is searched for in the EXE directory before the symbol
>> search path is used.  At least, that's what it used to do, back when I used
>> VS debugger for post-mortem debugging.  It was the only sane way to ensure
>> it would find the right version of the PDB if you didn't have a local
>> symbol server.
>>
>> Yes, I understand that the security note was about DLL loading.  My point
>> was that, in general, Windows looks in well known places first, before
>> checking the current working directory.
>>
>> On Tue, Dec 11, 2018 at 2:12 PM Leonard Mosescu 
>> wrote:
>>
>>> The Windowsy thing to do is what Zach said:  Check the directory that
 contains the .dmp for the .pdb.  It's the first place the VS debugger looks
 when opening a minidump.  It's less sensitive to the user's environment.
 (Making the user change the current working directory for this could be at
 odds with other bits of the software that look relative the cwd.)

 https://docs.microsoft.com/en-us/visualstudio/debugger/using-dump-files?view=vs-2017#BKMK_Find_binaries__symbol___pdb__files__and_source_files

>>>
>>> Except that it doesn't :) Neither VisualStudio nor the Windows Debuggers
>>> (windbg & co) look for PDBs in the same directory as the dump file. The
>>> search is controlled by an explicit "symbol search path". The link you
>>> mentioned is a bit confusing indeed (although it only claims that the
>>> .exe's are searched in the same directory as the .dmp)
>>>
>>> While security is not a big issue here, note that Windows generally
 searches for DLLs in the known/expected places _before_ checking to see if
 it's in the current working directory.  This prevents a sneaky download
 from effectively replacing a DLL.  Replacing a PDB is probably less of a
 concern.

 https://docs.microsoft.com/en-us/windows/desktop/dlls/dynamic-link-library-search-order#standard-search-order-for-desktop-applications

>>>
>>> This is about the runtime dynamic module loader, not the debugger.
>>>
>>>
>>>
>>>
>>> On Tue, Dec 11, 2018 at 1:28 PM Adrian McCarthy 
>>> wrote:
>>>
 It's really frustrating how the email discussion doesn't always make it
 to Phabricator.

 The Windowsy thing to do is what Zach said:  Check the directory that
 contains the .dmp for the .pdb.  It's the first place the VS debugger looks
 when opening a minidump.  It's less sensitive to the user's environment.
 (Making the user change the current working 

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

[Zachary Turner via lldb-commits]

Actually, Adrian just tested this on his machine and it did look in his
minidump folder.  I don't know why we're seeing different behavior.  Either
way, regardless of whether MSVC / WinDbg look in the minidump folder, I
still think it's a pretty intuitive location to add in the default search
path.  But if people disagree, it seems like target symbols add would still
address the problem in a permanent way.

On Tue, Dec 11, 2018 at 3:04 PM Zachary Turner  wrote:

> Only one way to know for sure, and that's to test it :)  So I did.
>
> Yes, it will search the directory of the EXE for the PDB.  But here, we're
> talking about a situation where there is no EXE, only a minidump.  If there
> is a minidump and no EXE then neither WinDbg nor VS will search the
> minidump folder for the PDB.  Personally I think it should and I wouldn't
> mind if that were a documented and tested feature of LLDB, because since it
> already is established behavior to search the minidump folder for the EXE,
> it doesn't seem hacky at all for it to also search that location for the
> PDB.  In my mind, the algorithm could be something like:
>
> ```
> if (ExeLocation = FindExecutableInMinidumpFolder()) {
>   PdbLocation = FindPdbFromExecutable(ExeLocation)
> } else {
>   PdbLocation = FindPdbInMinidumpFolder();
>   if (!PdbLocation)
> PdbLocation = FindPdbInSympath();
> }
> ```
>
> and that would be pretty logical and not code that we would have to
> consider temporary.
>
> My main pushback here is that it's harder to remove code than it is to add
> it, because once you add it, someone is going to depend on it and complain
> when you try to remove it.  So if we can just establish some behavior that
> satisfies the use case while not being temporary, I would prefer to do it.
>
> Another option I can think of is to just run `target symbols add -s
> foo.exe foo.pdb`.  I think we would need to have SymbolFileNativePDB check
> the value of this setting, but using this workflow, you could just put a
> .lldbinit file next to lldb.exe that runs this command at startup.
>
> On Tue, Dec 11, 2018 at 2:17 PM Adrian McCarthy 
> wrote:
>
>> I believe the PDB is searched for in the EXE directory before the symbol
>> search path is used.  At least, that's what it used to do, back when I used
>> VS debugger for post-mortem debugging.  It was the only sane way to ensure
>> it would find the right version of the PDB if you didn't have a local
>> symbol server.
>>
>> Yes, I understand that the security note was about DLL loading.  My point
>> was that, in general, Windows looks in well known places first, before
>> checking the current working directory.
>>
>> On Tue, Dec 11, 2018 at 2:12 PM Leonard Mosescu 
>> wrote:
>>
>>> The Windowsy thing to do is what Zach said:  Check the directory that
 contains the .dmp for the .pdb.  It's the first place the VS debugger looks
 when opening a minidump.  It's less sensitive to the user's environment.
 (Making the user change the current working directory for this could be at
 odds with other bits of the software that look relative the cwd.)

 https://docs.microsoft.com/en-us/visualstudio/debugger/using-dump-files?view=vs-2017#BKMK_Find_binaries__symbol___pdb__files__and_source_files

>>>
>>> Except that it doesn't :) Neither VisualStudio nor the Windows Debuggers
>>> (windbg & co) look for PDBs in the same directory as the dump file. The
>>> search is controlled by an explicit "symbol search path". The link you
>>> mentioned is a bit confusing indeed (although it only claims that the
>>> .exe's are searched in the same directory as the .dmp)
>>>
>>> While security is not a big issue here, note that Windows generally
 searches for DLLs in the known/expected places _before_ checking to see if
 it's in the current working directory.  This prevents a sneaky download
 from effectively replacing a DLL.  Replacing a PDB is probably less of a
 concern.

 https://docs.microsoft.com/en-us/windows/desktop/dlls/dynamic-link-library-search-order#standard-search-order-for-desktop-applications

>>>
>>> This is about the runtime dynamic module loader, not the debugger.
>>>
>>>
>>>
>>>
>>> On Tue, Dec 11, 2018 at 1:28 PM Adrian McCarthy 
>>> wrote:
>>>
 It's really frustrating how the email discussion doesn't always make it
 to Phabricator.

 The Windowsy thing to do is what Zach said:  Check the directory that
 contains the .dmp for the .pdb.  It's the first place the VS debugger looks
 when opening a minidump.  It's less sensitive to the user's environment.
 (Making the user change the current working directory for this could be at
 odds with other bits of the software that look relative the cwd.)


 https://docs.microsoft.com/en-us/visualstudio/debugger/using-dump-files?view=vs-2017#BKMK_Find_binaries__symbol___pdb__files__and_source_files

 While security is not a big issue here, note that Windows generally
 

[Lldb-commits] [PATCH] D55584: Simplify boolean expressions

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

I find the `static_cast` to be a bit too clever, I don't think it helps 
readability.

This is also too large to digest in a way I would feel satisfied that I did not 
miss anything.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55584



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

[Lldb-commits] [PATCH] D55574: Remove else statements after returns

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: source/Plugins/Process/Utility/RegisterContextDarwin_arm.cpp:994
 return FPURegSet;
   else if (reg < k_num_registers)
 return EXCRegSet;

This is inconsistent (do you need to re-run it until it reaches a fixpoint?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55574



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

[Lldb-commits] [PATCH] D55584: Simplify boolean expressions

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

This one is probably less controversial than D55574 
 :-)




Comment at: include/lldb/Target/ThreadPlanTracer.h:46
   TracingStarted();
-else if (old_value == true && value == false)
+else if (old_value && !value)
   TracingEnded();

is this actually better? Perhaps.



Comment at: source/DataFormatters/TypeCategoryMap.cpp:122
   Position p = First;
-  for (; false == m_active_categories.empty(); p++) {
+  for (; !m_active_categories.empty(); p++) {
 m_active_categories.front()->SetEnabledPosition(p);

this probably should be a while loop, or the initializer should go in the for.



Comment at: source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp:363
+if (!image->HasKey("load_address") || !image->HasKey("pathname") ||
+!image->HasKey("mod_date") || !image->HasKey("mach_header") ||
 image->GetValueForKey("mach_header")->GetAsDictionary() == nullptr ||

here the indentation actually got worse


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55584



___
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]

Only one way to know for sure, and that's to test it :)  So I did.

Yes, it will search the directory of the EXE for the PDB.  But here, we're
talking about a situation where there is no EXE, only a minidump.  If there
is a minidump and no EXE then neither WinDbg nor VS will search the
minidump folder for the PDB.  Personally I think it should and I wouldn't
mind if that were a documented and tested feature of LLDB, because since it
already is established behavior to search the minidump folder for the EXE,
it doesn't seem hacky at all for it to also search that location for the
PDB.  In my mind, the algorithm could be something like:

```
if (ExeLocation = FindExecutableInMinidumpFolder()) {
  PdbLocation = FindPdbFromExecutable(ExeLocation)
} else {
  PdbLocation = FindPdbInMinidumpFolder();
  if (!PdbLocation)
PdbLocation = FindPdbInSympath();
}
```

and that would be pretty logical and not code that we would have to
consider temporary.

My main pushback here is that it's harder to remove code than it is to add
it, because once you add it, someone is going to depend on it and complain
when you try to remove it.  So if we can just establish some behavior that
satisfies the use case while not being temporary, I would prefer to do it.

Another option I can think of is to just run `target symbols add -s foo.exe
foo.pdb`.  I think we would need to have SymbolFileNativePDB check the
value of this setting, but using this workflow, you could just put a
.lldbinit file next to lldb.exe that runs this command at startup.

On Tue, Dec 11, 2018 at 2:17 PM Adrian McCarthy  wrote:

> I believe the PDB is searched for in the EXE directory before the symbol
> search path is used.  At least, that's what it used to do, back when I used
> VS debugger for post-mortem debugging.  It was the only sane way to ensure
> it would find the right version of the PDB if you didn't have a local
> symbol server.
>
> Yes, I understand that the security note was about DLL loading.  My point
> was that, in general, Windows looks in well known places first, before
> checking the current working directory.
>
> On Tue, Dec 11, 2018 at 2:12 PM Leonard Mosescu 
> wrote:
>
>> The Windowsy thing to do is what Zach said:  Check the directory that
>>> contains the .dmp for the .pdb.  It's the first place the VS debugger looks
>>> when opening a minidump.  It's less sensitive to the user's environment.
>>> (Making the user change the current working directory for this could be at
>>> odds with other bits of the software that look relative the cwd.)
>>>
>>> https://docs.microsoft.com/en-us/visualstudio/debugger/using-dump-files?view=vs-2017#BKMK_Find_binaries__symbol___pdb__files__and_source_files
>>>
>>
>> Except that it doesn't :) Neither VisualStudio nor the Windows Debuggers
>> (windbg & co) look for PDBs in the same directory as the dump file. The
>> search is controlled by an explicit "symbol search path". The link you
>> mentioned is a bit confusing indeed (although it only claims that the
>> .exe's are searched in the same directory as the .dmp)
>>
>> While security is not a big issue here, note that Windows generally
>>> searches for DLLs in the known/expected places _before_ checking to see if
>>> it's in the current working directory.  This prevents a sneaky download
>>> from effectively replacing a DLL.  Replacing a PDB is probably less of a
>>> concern.
>>>
>>> https://docs.microsoft.com/en-us/windows/desktop/dlls/dynamic-link-library-search-order#standard-search-order-for-desktop-applications
>>>
>>
>> This is about the runtime dynamic module loader, not the debugger.
>>
>>
>>
>>
>> On Tue, Dec 11, 2018 at 1:28 PM Adrian McCarthy 
>> wrote:
>>
>>> It's really frustrating how the email discussion doesn't always make it
>>> to Phabricator.
>>>
>>> The Windowsy thing to do is what Zach said:  Check the directory that
>>> contains the .dmp for the .pdb.  It's the first place the VS debugger looks
>>> when opening a minidump.  It's less sensitive to the user's environment.
>>> (Making the user change the current working directory for this could be at
>>> odds with other bits of the software that look relative the cwd.)
>>>
>>>
>>> https://docs.microsoft.com/en-us/visualstudio/debugger/using-dump-files?view=vs-2017#BKMK_Find_binaries__symbol___pdb__files__and_source_files
>>>
>>> While security is not a big issue here, note that Windows generally
>>> searches for DLLs in the known/expected places _before_ checking to see if
>>> it's in the current working directory.  This prevents a sneaky download
>>> from effectively replacing a DLL.  Replacing a PDB is probably less of a
>>> concern.
>>>
>>>
>>> https://docs.microsoft.com/en-us/windows/desktop/dlls/dynamic-link-library-search-order#standard-search-order-for-desktop-applications
>>>
>>> So I +1 Zach's proposal.
>>>
>>> On Tue, Dec 11, 2018 at 12:07 PM Leonard Mosescu 
>>> wrote:
>>>
 I think as combination of explicit symbol search path + something
 similar to Microsoft's 

Re: [Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

[Zachary Turner via lldb-commits]

On Tue, Dec 11, 2018 at 11:57 AM Pavel Labath  wrote:

> The part I know nothing about is whether something similar could be done
> for PE/COFF files (and I'll need something like that there too). Adrian,
> Zachary, what is the relation ship between "image base" of an object
> file and its sections? Is there any way we could arrange so that the
> base address of a module always belongs to one of its sections?
>
>
Historically, an image base of N was used as a way to tell the loader "map
the file in so that byte 0 of the file is virtual address N in the
process's address space".  as in *((char *)N) would be the first byte of
the file in a running process.  Then, everything else in the file is
written as an offset from N.  This includes section addresses.  So for
example, if we use dumpbin on a simple executable we can see something like
this:

Dump of file bin\count.exe

PE signature found

File Type: EXECUTABLE IMAGE

OPTIONAL HEADER VALUES
  ...
   14000 image base (00014000 to 000140011FFF)
  ...
SECTION HEADER #1
   .text name
1000 virtual address (000140001000 to 0001400089AE)

So under this scheme, the first byte of the first section would be at
virtual address 000140001000 in the running process.

Later, ASLR came along and threw a wrench in all of that, and so these days
the image base is mostly meaningless.  The loader will probably never
actually load your module at the address specified in image base.  But the
rest of the rules still hold true.  Wherever it *does* load your module,
the first byte of .text will still be at offset 1000 from that.

So, if you want to return this value from the PE/COFF header, or even if
you want to return the actual address that the module was loaded at, then
no, it will never belong to any section (because the bytes at that address
will be the PE/COFF file header).

Does this make sense?
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55574: Remove else statements after returns

[Vedant Kumar via Phabricator via lldb-commits]

vsk added a comment.

+ 1 to this. If there's a tidy plugin for misleading indention, that might 
address some of Adrian's concerns.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55574



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

[Lldb-commits] [PATCH] D55574: Remove else statements after returns

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

I'm mostly on board with making these changes, as it's good LLVM style to do 
this. I highlighted a couple changes that might warrant a closer look.




Comment at: source/API/SBProcess.cpp:1233
 error.SetErrorString("process is invalid");
   }
+

This function probably could use some manual refactoring, too.



Comment at: source/Commands/CommandObjectTarget.cpp:2569
+  }
+  StreamString strm;
+  module_spec.GetUUID().Dump();

Can you manually double-check this one?



Comment at: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:3818
+static_cast(namespace_decl->getOriginalNamespace()));
+  }
   }

what's going on here with the indentation?



Comment at: source/Target/Process.cpp:1930
+error.AsCString() ? error.AsCString() : "unknown error");
+  }
+  }

this also looks suspicious



Comment at: source/Target/Process.cpp:5235
+  event_to_broadcast_sp, options, handle_interrupts);
+}
 } break;

wouldn't hurt to manually check this one, too



Comment at: source/Target/ThreadPlanStepRange.cpp:329
+  run_to_address.Slide(last_inst_size);
+}
 } else if (branch_index - pc_index > 1) {

weird indentation again



Comment at: source/Target/ThreadPlanStepUntil.cpp:246
 }
-  }
-  // If we get here we haven't hit any of our breakpoints, so let the
-  // higher plans take care of the stop.
-  m_explains_stop = false;
-  return;
-} else if (IsUsuallyUnexplainedStopReason(reason)) {
+}
+

again


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55574



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

[Lldb-commits] [lldb] r348901 - Remove unused file

[Jonas Devlieghere via lldb-commits]

Author: jdevlieghere
Date: Tue Dec 11 14:46:56 2018
New Revision: 348901

URL: http://llvm.org/viewvc/llvm-project?rev=348901=rev
Log:
Remove unused file

I removed the dotest-style reproducer test but forgot to delete the
source file. Thanks Jim for the heads up!

Removed:

lldb/trunk/packages/Python/lldbsuite/test/functionalities/reproducer/gdb-remote/main.c

Removed: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/reproducer/gdb-remote/main.c
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/reproducer/gdb-remote/main.c?rev=348900=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/reproducer/gdb-remote/main.c
 (original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/reproducer/gdb-remote/main.c
 (removed)
@@ -1,19 +0,0 @@
-//===-- main.c --*- C++ 
-*-===//
-//
-// The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===--===//
-
-#include 
-
-void foo() {
-printf("testing\n");
-}
-
-int main (int argc, char const *argv[]) {
-foo();
-return 0;
-}


___
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 created this revision.
JDevlieghere added reviewers: labath, jingham, sgraenitz.
JDevlieghere added a project: LLDB.
Herald added a subscriber: teemperor.

Add support for reproducing to the command interpreter. In capture mode we 
capture all the command to file and in reproducer mode we replay this.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D55582

Files:
  include/lldb/API/SBDebugger.h
  include/lldb/Core/Debugger.h
  include/lldb/Interpreter/CommandInterpreter.h
  lit/Reproducer/TestCommandRepro.test
  source/API/SBDebugger.cpp
  source/Core/Debugger.cpp
  source/Interpreter/CommandInterpreter.cpp
  tools/driver/Driver.cpp
  tools/driver/Driver.h

Index: tools/driver/Driver.h
===
--- tools/driver/Driver.h
+++ tools/driver/Driver.h
@@ -100,6 +100,7 @@
 bool m_wait_for = false;
 bool m_repl = false;
 bool m_batch = false;
+bool m_replay = false;
 
 // FIXME: When we have set/show variables we can remove this from here.
 bool m_use_external_editor = false;
Index: tools/driver/Driver.cpp
===
--- tools/driver/Driver.cpp
+++ tools/driver/Driver.cpp
@@ -274,6 +274,10 @@
 m_option_data.m_batch = true;
   }
 
+  if (auto *arg = args.getLastArg(OPT_replay)) {
+m_option_data.m_replay = true;
+  }
+
   if (auto *arg = args.getLastArg(OPT_core)) {
 auto arg_value = arg->getValue();
 SBFileSpec file(arg_value);
@@ -677,6 +681,8 @@
   else
 WithColor::error() << error.GetError() << '\n';
 }
+  } else if (m_option_data.m_replay) {
+m_debugger.RunReplay();
   } else {
 // Check if we have any data in the commands stream, and if so, save it to a
 // temp file
Index: source/Interpreter/CommandInterpreter.cpp
===
--- source/Interpreter/CommandInterpreter.cpp
+++ source/Interpreter/CommandInterpreter.cpp
@@ -45,6 +45,7 @@
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Utility/Log.h"
+#include "lldb/Utility/Reproducer.h"
 #include "lldb/Utility/State.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/Timer.h"
@@ -74,6 +75,7 @@
 
 using namespace lldb;
 using namespace lldb_private;
+using namespace llvm;
 
 static const char *k_white_space = " \t\v";
 
@@ -116,6 +118,46 @@
   eEchoCommentCommands = 5
 };
 
+class lldb_private::CommandProvider
+: public repro::Provider {
+public:
+  CommandProvider(const FileSpec ) : Provider(directory) {
+m_info.name = "command-interpreter";
+m_info.files.push_back("command-interpreter.txt");
+  }
+
+  void CaptureCommand(std::string command) {
+// Explicitly ignore reproducer commands.
+if (command.find("reproducer") == 0)
+  return;
+
+m_commands.push_back(std::move(command));
+  }
+
+  void Keep() override {
+FileSpec file =
+GetRoot().CopyByAppendingPathComponent("command-interpreter.txt");
+
+std::error_code ec;
+llvm::raw_fd_ostream os(file.GetPath(), ec, llvm::sys::fs::F_Text);
+
+if (ec)
+  return;
+
+for (auto  : m_commands)
+  os << command << '\n';
+  }
+
+  void Discard() override {}
+
+  static char ID;
+
+private:
+  std::vector m_commands;
+};
+
+char CommandProvider::ID = 0;
+
 ConstString ::GetStaticBroadcasterClass() {
   static ConstString class_name("lldb.commandInterpreter");
   return class_name;
@@ -141,6 +183,9 @@
   SetEventName(eBroadcastBitQuitCommandReceived, "quit");
   CheckInWithManager();
   m_collection_sp->Initialize(g_properties);
+
+  if (repro::Generator *g = repro::Reproducer::Instance().GetGenerator())
+m_provider = >GetOrCreate();
 }
 
 bool CommandInterpreter::GetExpandRegexAliases() const {
@@ -1679,6 +1724,9 @@
 
   Status error(PreprocessCommand(command_string));
 
+  if (m_provider)
+m_provider->CaptureCommand(original_command_string);
+
   if (error.Fail()) {
 result.AppendError(error.AsCString());
 result.SetStatus(eReturnStatusFailed);
@@ -2074,6 +2122,45 @@
   return position;
 }
 
+void CommandInterpreter::ReplayCommands(CommandReturnObject ) {
+  repro::Loader *loader = repro::Reproducer::Instance().GetLoader();
+  if (!loader) {
+result.SetStatus(eReturnStatusSuccessFinishNoResult);
+return;
+  }
+
+  auto provider_info = loader->GetProviderInfo("command-interpreter");
+  if (!provider_info) {
+result.AppendErrorWithFormat("no provider for command interpreter.");
+result.SetStatus(eReturnStatusFailed);
+return;
+  }
+
+  if (provider_info->files.empty()) {
+result.AppendErrorWithFormat(
+"provider for command interpreter contains no files.");
+result.SetStatus(eReturnStatusFailed);
+return;
+  }
+
+  FileSpec command_file = loader->GetRoot().CopyByAppendingPathComponent(
+  provider_info->files.front());
+
+  // Remember current batch mode.
+  const bool saved_batch = 

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

[Greg Clayton via Phabricator via lldb-commits]

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

Looks good to me. Jim should ok as well.


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

[Adrian McCarthy via lldb-commits]

I believe the PDB is searched for in the EXE directory before the symbol
search path is used.  At least, that's what it used to do, back when I used
VS debugger for post-mortem debugging.  It was the only sane way to ensure
it would find the right version of the PDB if you didn't have a local
symbol server.

Yes, I understand that the security note was about DLL loading.  My point
was that, in general, Windows looks in well known places first, before
checking the current working directory.

On Tue, Dec 11, 2018 at 2:12 PM Leonard Mosescu  wrote:

> The Windowsy thing to do is what Zach said:  Check the directory that
>> contains the .dmp for the .pdb.  It's the first place the VS debugger looks
>> when opening a minidump.  It's less sensitive to the user's environment.
>> (Making the user change the current working directory for this could be at
>> odds with other bits of the software that look relative the cwd.)
>>
>> https://docs.microsoft.com/en-us/visualstudio/debugger/using-dump-files?view=vs-2017#BKMK_Find_binaries__symbol___pdb__files__and_source_files
>>
>
> Except that it doesn't :) Neither VisualStudio nor the Windows Debuggers
> (windbg & co) look for PDBs in the same directory as the dump file. The
> search is controlled by an explicit "symbol search path". The link you
> mentioned is a bit confusing indeed (although it only claims that the
> .exe's are searched in the same directory as the .dmp)
>
> While security is not a big issue here, note that Windows generally
>> searches for DLLs in the known/expected places _before_ checking to see if
>> it's in the current working directory.  This prevents a sneaky download
>> from effectively replacing a DLL.  Replacing a PDB is probably less of a
>> concern.
>>
>> https://docs.microsoft.com/en-us/windows/desktop/dlls/dynamic-link-library-search-order#standard-search-order-for-desktop-applications
>>
>
> This is about the runtime dynamic module loader, not the debugger.
>
>
>
>
> On Tue, Dec 11, 2018 at 1:28 PM Adrian McCarthy 
> wrote:
>
>> It's really frustrating how the email discussion doesn't always make it
>> to Phabricator.
>>
>> The Windowsy thing to do is what Zach said:  Check the directory that
>> contains the .dmp for the .pdb.  It's the first place the VS debugger looks
>> when opening a minidump.  It's less sensitive to the user's environment.
>> (Making the user change the current working directory for this could be at
>> odds with other bits of the software that look relative the cwd.)
>>
>>
>> https://docs.microsoft.com/en-us/visualstudio/debugger/using-dump-files?view=vs-2017#BKMK_Find_binaries__symbol___pdb__files__and_source_files
>>
>> While security is not a big issue here, note that Windows generally
>> searches for DLLs in the known/expected places _before_ checking to see if
>> it's in the current working directory.  This prevents a sneaky download
>> from effectively replacing a DLL.  Replacing a PDB is probably less of a
>> concern.
>>
>>
>> https://docs.microsoft.com/en-us/windows/desktop/dlls/dynamic-link-library-search-order#standard-search-order-for-desktop-applications
>>
>> So I +1 Zach's proposal.
>>
>> On Tue, Dec 11, 2018 at 12:07 PM Leonard Mosescu 
>> wrote:
>>
>>> I think as combination of explicit symbol search path + something
>>> similar to Microsoft's symsrv would be a good "real" solution (and yes,
>>> that would be packaged as a SymbolVendor, outside SymbolFilePDB)
>>>
>>> For short term, I don't see a clearly superior alternative to searching
>>> the current directory.
>>>
>>> On Tue, Dec 11, 2018 at 12:02 PM Pavel Labath  wrote:
>>>
 On 11/12/2018 20:34, Zachary Turner wrote:
 > I meant the location of the minidump.  So if you have
 C:\A\B\C\foo.dmp
 > which is the dump file for bar.exe which crashed on another machine,
 > then it would look for C:\A\B\C\bar.pdb.  That actually seems like
 > fairly intuitive behavior to me, but maybe I'm in the minority :)
 >
 > We can see what Pavel, Adrian, and others think though or if they
 have
 > any other suggestions.
 >

 It sounds like there is a precedent for searching in CWD. I don't know
 how useful it is (I traced it back to r185366, but it is not mentioned
 there specifically), but it is there, and I guess it's not completely
 nonsensical from the POV of a command line user.

 I guess we can just keep that there and not call it a hack (though, the
 fact that the searching happens inside SymbolFilePDB *is* a hack).

 Searching in the minidump directory would also make sense somewhat, but
 I expect you would need more plumbing for that to happen (and I don't
 know of a precedent for that).

 pl

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

Re: [Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

[Greg Clayton via lldb-commits]

> On Dec 11, 2018, at 11:58 AM, Pavel Labath  wrote:
> 
> On 11/12/2018 20:39, Jim Ingham wrote:
>> Sections can have parents.  In MachO the text and data sections are actually 
>> contained in the TEXT and DATA segments respectively.  LLDB represents this 
>> by having an lldb_private::Section for the segment, and then all the 
>> sections in that segment are children of the parent Section (the MachO 
>> segment).  All the code that looks up addresses expects to potentially 
>> traverse this hierarchy.
>> It seems to me that what you are describing should fit in the nesting model?
>> Jim
>>  
> 
> Hmm.. that's an interesting idea. I'll have to think about that.
> 

I think having the program header segments being top level sections in ELF, 
with the sections headers contained within them would fix things correctly and 
allow us to give out as section + offset base address. 

> Does MachO enforce somehow that every section is fully contained in one 
> segment?

yes


> Because while that is true for elf in general, it is not a requirement, and 
> it is possible to create functional executables (though I think I would have 
> to use a hex editor for that) where one segment contains only a half of some 
> section. But that might not be a big issue, as I think most tools will choke 
> on files like this, so if we do something suboptimal here, it shouldn't be 
> too bad.

I would be surprised to see this kind of thing, though it could exist. We can 
easily just make two top level sections when this happens so it can remain 
consistent?

> 
> The other hickup I can think of is that sometimes (and this happens in 
> perfectly valid files too) one section is present in multiple segments. For 
> example .eh_frame will be present in a "LOAD" segment (so it ends up in 
> memory), but it will also be present in a special "GNU_EH_FRAME" segment (not 
> sure why, but I guess it's to make it easier for someone to find it). But I 
> think I can get around that by only creating these "container" sections for 
> LOAD segments.

I agree, LOAD segments would be created, and any sections that would fit inside 
of those will be put there, otherwise, they are top level sections?
> 
> 
> The part I know nothing about is whether something similar could be done for 
> PE/COFF files (and I'll need something like that there too). Adrian, Zachary, 
> what is the relation ship between "image base" of an object file and its 
> sections? Is there any way we could arrange so that the base address of a 
> module always belongs to one of its sections?

That is the key question.

> 
> pl

___
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]

>
> The Windowsy thing to do is what Zach said:  Check the directory that
> contains the .dmp for the .pdb.  It's the first place the VS debugger looks
> when opening a minidump.  It's less sensitive to the user's environment.
> (Making the user change the current working directory for this could be at
> odds with other bits of the software that look relative the cwd.)
>
> https://docs.microsoft.com/en-us/visualstudio/debugger/using-dump-files?view=vs-2017#BKMK_Find_binaries__symbol___pdb__files__and_source_files
>

Except that it doesn't :) Neither VisualStudio nor the Windows Debuggers
(windbg & co) look for PDBs in the same directory as the dump file. The
search is controlled by an explicit "symbol search path". The link you
mentioned is a bit confusing indeed (although it only claims that the
.exe's are searched in the same directory as the .dmp)

While security is not a big issue here, note that Windows generally
> searches for DLLs in the known/expected places _before_ checking to see if
> it's in the current working directory.  This prevents a sneaky download
> from effectively replacing a DLL.  Replacing a PDB is probably less of a
> concern.
>
> https://docs.microsoft.com/en-us/windows/desktop/dlls/dynamic-link-library-search-order#standard-search-order-for-desktop-applications
>

This is about the runtime dynamic module loader, not the debugger.




On Tue, Dec 11, 2018 at 1:28 PM Adrian McCarthy  wrote:

> It's really frustrating how the email discussion doesn't always make it to
> Phabricator.
>
> The Windowsy thing to do is what Zach said:  Check the directory that
> contains the .dmp for the .pdb.  It's the first place the VS debugger looks
> when opening a minidump.  It's less sensitive to the user's environment.
> (Making the user change the current working directory for this could be at
> odds with other bits of the software that look relative the cwd.)
>
>
> https://docs.microsoft.com/en-us/visualstudio/debugger/using-dump-files?view=vs-2017#BKMK_Find_binaries__symbol___pdb__files__and_source_files
>
> While security is not a big issue here, note that Windows generally
> searches for DLLs in the known/expected places _before_ checking to see if
> it's in the current working directory.  This prevents a sneaky download
> from effectively replacing a DLL.  Replacing a PDB is probably less of a
> concern.
>
>
> https://docs.microsoft.com/en-us/windows/desktop/dlls/dynamic-link-library-search-order#standard-search-order-for-desktop-applications
>
> So I +1 Zach's proposal.
>
> On Tue, Dec 11, 2018 at 12:07 PM Leonard Mosescu 
> wrote:
>
>> I think as combination of explicit symbol search path + something similar
>> to Microsoft's symsrv would be a good "real" solution (and yes, that would
>> be packaged as a SymbolVendor, outside SymbolFilePDB)
>>
>> For short term, I don't see a clearly superior alternative to searching
>> the current directory.
>>
>> On Tue, Dec 11, 2018 at 12:02 PM Pavel Labath  wrote:
>>
>>> On 11/12/2018 20:34, Zachary Turner wrote:
>>> > I meant the location of the minidump.  So if you have C:\A\B\C\foo.dmp
>>> > which is the dump file for bar.exe which crashed on another machine,
>>> > then it would look for C:\A\B\C\bar.pdb.  That actually seems like
>>> > fairly intuitive behavior to me, but maybe I'm in the minority :)
>>> >
>>> > We can see what Pavel, Adrian, and others think though or if they have
>>> > any other suggestions.
>>> >
>>>
>>> It sounds like there is a precedent for searching in CWD. I don't know
>>> how useful it is (I traced it back to r185366, but it is not mentioned
>>> there specifically), but it is there, and I guess it's not completely
>>> nonsensical from the POV of a command line user.
>>>
>>> I guess we can just keep that there and not call it a hack (though, the
>>> fact that the searching happens inside SymbolFilePDB *is* a hack).
>>>
>>> Searching in the minidump directory would also make sense somewhat, but
>>> I expect you would need more plumbing for that to happen (and I don't
>>> know of a precedent for that).
>>>
>>> pl
>>>
>>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55569: [lit] Add a basic implementation of build for GccBuilder

[Stella Stamenova via Phabricator via lldb-commits]

stella.stamenova abandoned this revision.
stella.stamenova added a comment.

Let's use @labath's change instead.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55569



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

[Lldb-commits] [PATCH] D55430: build.py: Implement "gcc" builder

[Stella Stamenova via Phabricator via lldb-commits]

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

A couple of small comments, but it looks good otherwise. Thanks!




Comment at: lit/BuildScript/toolchain-clang.test:1
+RUN: %build -n --verbose --arch=32 --compiler=clang --mode=compile-and-link -o 
%t/foo.exe foobar.c \
+RUN:| FileCheck --check-prefix=CHECK --check-prefix=CHECK-32 %s

There are a couple of tests for the build script that we should enable on 
non-windows as well:
* script-args
* modes
Both should pass with your change



Comment at: lit/helper/build.py:627
+elif self.opt == 'lto':
+raise NotImplementedError("LTO not implemented in gcc builder")
+if self.nodefaultlib:

You can just add the thinlto command here. That should give you parity with the 
msvc builder:

args.append('-flto=thin')





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

https://reviews.llvm.org/D55430



___
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

[Adrian McCarthy via lldb-commits]

It's really frustrating how the email discussion doesn't always make it to
Phabricator.

The Windowsy thing to do is what Zach said:  Check the directory that
contains the .dmp for the .pdb.  It's the first place the VS debugger looks
when opening a minidump.  It's less sensitive to the user's environment.
(Making the user change the current working directory for this could be at
odds with other bits of the software that look relative the cwd.)

https://docs.microsoft.com/en-us/visualstudio/debugger/using-dump-files?view=vs-2017#BKMK_Find_binaries__symbol___pdb__files__and_source_files

While security is not a big issue here, note that Windows generally
searches for DLLs in the known/expected places _before_ checking to see if
it's in the current working directory.  This prevents a sneaky download
from effectively replacing a DLL.  Replacing a PDB is probably less of a
concern.

https://docs.microsoft.com/en-us/windows/desktop/dlls/dynamic-link-library-search-order#standard-search-order-for-desktop-applications

So I +1 Zach's proposal.

On Tue, Dec 11, 2018 at 12:07 PM Leonard Mosescu  wrote:

> I think as combination of explicit symbol search path + something similar
> to Microsoft's symsrv would be a good "real" solution (and yes, that would
> be packaged as a SymbolVendor, outside SymbolFilePDB)
>
> For short term, I don't see a clearly superior alternative to searching
> the current directory.
>
> On Tue, Dec 11, 2018 at 12:02 PM Pavel Labath  wrote:
>
>> On 11/12/2018 20:34, Zachary Turner wrote:
>> > I meant the location of the minidump.  So if you have C:\A\B\C\foo.dmp
>> > which is the dump file for bar.exe which crashed on another machine,
>> > then it would look for C:\A\B\C\bar.pdb.  That actually seems like
>> > fairly intuitive behavior to me, but maybe I'm in the minority :)
>> >
>> > We can see what Pavel, Adrian, and others think though or if they have
>> > any other suggestions.
>> >
>>
>> It sounds like there is a precedent for searching in CWD. I don't know
>> how useful it is (I traced it back to r185366, but it is not mentioned
>> there specifically), but it is there, and I guess it's not completely
>> nonsensical from the POV of a command line user.
>>
>> I guess we can just keep that there and not call it a hack (though, the
>> fact that the searching happens inside SymbolFilePDB *is* a hack).
>>
>> Searching in the minidump directory would also make sense somewhat, but
>> I expect you would need more plumbing for that to happen (and I don't
>> know of a precedent for that).
>>
>> pl
>>
>
___
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

[Zachary Turner via Phabricator via lldb-commits]

zturner created this revision.
zturner added reviewers: labath, lemo, aleksandr.urakov, amccarth, clayborg, 
leonid.mashinskiy.
Herald added subscribers: JDevlieghere, aprantl.

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

While implementing this, I came up with several new ideas for increasing test 
coverage throughout LLDB (not limited to this plugin), including:

- We should start extending AST tests to other types of targets and debug info, 
not just windows / native pdb.  This uncovered a few bugs for me here that 
allowed me to proceed.
- We should have comprehensive test coverage of the DWARFExpression evaluator 
in lldb/Expression/DWARFExpression.cpp.  This would allow us to improve test 
coverage of scenarios that require compilers to emit specific debug info, and 
allow us to do it in a way that doesn't require that debug info to have been 
emitted by some compiler.
- We should add options to `lldb-test` that will allow it to call into 
`SymbolFile::ResolveSymbolContext` and dump the output in some format that is 
interesting for file checking.  This function is at the heart of a lot of 
logic, and if it doesn't work, many things will fail.  Being able to test all 
of the edge cases of this function would weed out a lot of problems.

Anyway, for now these are all just ideas.  For this patch, I've just 
implemented support for local variables and a test to make sure it works.  One 
interesting thing: With the old PDB plugin, this test takes over 1 minute to 
run.  With the new PDB plugin, it takes about 2 seconds.


https://reviews.llvm.org/D55575

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

Index: llvm/include/llvm/Support/BinaryStreamArray.h
===
--- llvm/include/llvm/Support/BinaryStreamArray.h
+++ llvm/include/llvm/Support/BinaryStreamArray.h
@@ -139,7 +139,7 @@
 this->Skew = Skew;
   }
 
-  void drop_front() { Stream = Stream.drop_front(begin()->length()); }
+  void drop_front() { Skew += begin()->length(); }
 
 private:
   BinaryStreamRef Stream;
Index: lldb/source/Symbol/ClangASTContext.cpp
===
--- lldb/source/Symbol/ClangASTContext.cpp
+++ lldb/source/Symbol/ClangASTContext.cpp
@@ -2218,10 +2218,13 @@
 const CompilerType _type, int storage) {
   ASTContext *ast = getASTContext();
   assert(ast != nullptr);
-  return ParmVarDecl::Create(*ast, decl_ctx, SourceLocation(), SourceLocation(),
- name && name[0] ? >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] ? >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: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
@@ -109,10 +109,10 @@
VariableList ) override;
 
   size_t ParseTypes(const SymbolContext ) override;
-  size_t ParseVariablesForContext(const SymbolContext ) override {
-return 0;
-  }
+  size_t ParseVariablesForContext(const SymbolContext ) 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 );
+   CompileUnit _unit);
   lldb::CompUnitSP GetOrCreateCompileUnit(const CompilandIndexItem );
   lldb::TypeSP GetOrCreateType(PdbTypeSymId type_id);
   

[Lldb-commits] [PATCH] D55574: Remove else statements after returns

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added a reviewer: LLDB.
Herald added subscribers: jsji, teemperor, abidh, arphaman, atanasyan, kbarton, 
arichardson, javed.absar, ki.stfu, nemanjai, kubamracek, sdardis, emaste, 
srhines.
Herald added a reviewer: espindola.
Herald added a reviewer: jfb.
Herald added a reviewer: shafik.

While writing a patch I noticed myself removing a few `else` statements after 
`return` statements. Rather than doing this ad-hoc I remembered there's a 
clang-tidy pass that does this.

https://clang.llvm.org/extra/clang-tidy/checks/readability-else-after-return.html

So essentially what I did was run the following command over the LLDB code base:

  run-clang-tidy.py -checks='-*,readability-else-after-return' -format -fix $PWD

I'm not sure if it's worth the churn, but I do believe lldb could benefit from 
some reduced indentation.

PS: Diff is without context, otherwise it exceeds the Phabricator limit of 8 
megabytes.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D55574

Files:
  include/lldb/Breakpoint/BreakpointResolver.h
  include/lldb/Core/Broadcaster.h
  include/lldb/Core/Event.h
  include/lldb/Core/FileSpecList.h
  include/lldb/Core/RangeMap.h
  include/lldb/Core/SearchFilter.h
  include/lldb/Core/StructuredDataImpl.h
  include/lldb/Core/ValueObject.h
  include/lldb/DataFormatters/FormattersContainer.h
  include/lldb/DataFormatters/FormattersHelpers.h
  include/lldb/DataFormatters/TypeCategory.h
  include/lldb/Host/ProcessRunLock.h
  include/lldb/Interpreter/CommandObject.h
  include/lldb/Symbol/ClangASTImporter.h
  include/lldb/Symbol/ClangExternalASTSourceCommon.h
  include/lldb/Symbol/DebugMacros.h
  include/lldb/Symbol/Symbol.h
  include/lldb/Symbol/Type.h
  include/lldb/Target/ObjCLanguageRuntime.h
  include/lldb/Target/Process.h
  include/lldb/Target/StackFrameList.h
  include/lldb/Target/StopInfo.h
  include/lldb/Target/ThreadPlan.h
  include/lldb/Target/ThreadPlanCallFunction.h
  include/lldb/Target/ThreadSpec.h
  source/API/SBAddress.cpp
  source/API/SBBreakpoint.cpp
  source/API/SBBreakpointLocation.cpp
  source/API/SBBreakpointName.cpp
  source/API/SBCommandReturnObject.cpp
  source/API/SBDebugger.cpp
  source/API/SBEvent.cpp
  source/API/SBInstruction.cpp
  source/API/SBListener.cpp
  source/API/SBProcess.cpp
  source/API/SBSourceManager.cpp
  source/API/SBThread.cpp
  source/API/SBThreadPlan.cpp
  source/API/SBTypeCategory.cpp
  source/API/SBTypeFilter.cpp
  source/API/SBTypeFormat.cpp
  source/API/SBTypeSummary.cpp
  source/API/SBTypeSynthetic.cpp
  source/API/SBValue.cpp
  source/API/SBWatchpoint.cpp
  source/Breakpoint/Breakpoint.cpp
  source/Breakpoint/BreakpointIDList.cpp
  source/Breakpoint/BreakpointLocation.cpp
  source/Breakpoint/BreakpointLocationList.cpp
  source/Breakpoint/BreakpointOptions.cpp
  source/Breakpoint/BreakpointResolver.cpp
  source/Breakpoint/BreakpointResolverName.cpp
  source/Breakpoint/BreakpointResolverScripted.cpp
  source/Breakpoint/BreakpointSiteList.cpp
  source/Breakpoint/Watchpoint.cpp
  source/Breakpoint/WatchpointOptions.cpp
  source/Commands/CommandCompletions.cpp
  source/Commands/CommandObjectBreakpoint.cpp
  source/Commands/CommandObjectCommands.cpp
  source/Commands/CommandObjectDisassemble.cpp
  source/Commands/CommandObjectFrame.cpp
  source/Commands/CommandObjectHelp.cpp
  source/Commands/CommandObjectMemory.cpp
  source/Commands/CommandObjectMultiword.cpp
  source/Commands/CommandObjectPlatform.cpp
  source/Commands/CommandObjectProcess.cpp
  source/Commands/CommandObjectSettings.cpp
  source/Commands/CommandObjectSource.cpp
  source/Commands/CommandObjectTarget.cpp
  source/Commands/CommandObjectThread.cpp
  source/Commands/CommandObjectType.cpp
  source/Commands/CommandObjectWatchpoint.cpp
  source/Core/Address.cpp
  source/Core/AddressRange.cpp
  source/Core/Broadcaster.cpp
  source/Core/Debugger.cpp
  source/Core/Disassembler.cpp
  source/Core/DumpDataExtractor.cpp
  source/Core/DynamicLoader.cpp
  source/Core/Event.cpp
  source/Core/FormatEntity.cpp
  source/Core/IOHandler.cpp
  source/Core/Listener.cpp
  source/Core/Mangled.cpp
  source/Core/Module.cpp
  source/Core/ModuleList.cpp
  source/Core/PluginManager.cpp
  source/Core/SearchFilter.cpp
  source/Core/Section.cpp
  source/Core/SourceManager.cpp
  source/Core/Value.cpp
  source/Core/ValueObject.cpp
  source/Core/ValueObjectConstResultImpl.cpp
  source/Core/ValueObjectDynamicValue.cpp
  source/Core/ValueObjectRegister.cpp
  source/Core/ValueObjectSyntheticFilter.cpp
  source/Core/ValueObjectVariable.cpp
  source/DataFormatters/CXXFunctionPointer.cpp
  source/DataFormatters/FormatManager.cpp
  source/DataFormatters/TypeCategory.cpp
  source/DataFormatters/TypeFormat.cpp
  source/DataFormatters/TypeSummary.cpp
  source/DataFormatters/ValueObjectPrinter.cpp
  source/Expression/DWARFExpression.cpp
  source/Expression/ExpressionSourceCode.cpp
  source/Expression/ExpressionVariable.cpp
  source/Expression/IRDynamicChecks.cpp
  

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

[Zachary Turner via Phabricator via lldb-commits]

zturner created this revision.
zturner added reviewers: clayborg, jingham, labath, davide.
Herald added subscribers: JDevlieghere, kristof.beyls, javed.absar.

Previously CreateParameterDeclaration was always using the translation unit 
DeclContext.  We would later go and add parameters to the FunctionDecl, but 
internally clang makes a copy when you do this, and we'd end up with 
ParmVarDecl's at the global scope as well as in the function scope.

This fixes the issue.  It's hard to say whether this will introduce a 
behavioral change in name lookup, but I know there have been several hacks 
introduced in previous years to deal with collisions between various types of 
variables, so there's a chance that this patch could obviate one of those hacks.


https://reviews.llvm.org/D55571

Files:
  lldb/include/lldb/Symbol/ClangASTContext.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
  lldb/source/Symbol/ClangASTContext.cpp


Index: lldb/source/Symbol/ClangASTContext.cpp
===
--- lldb/source/Symbol/ClangASTContext.cpp
+++ lldb/source/Symbol/ClangASTContext.cpp
@@ -2214,11 +2214,11 @@
 }
 
 ParmVarDecl *ClangASTContext::CreateParameterDeclaration(
-const char *name, const CompilerType _type, int storage) {
+clang::DeclContext *decl_ctx, const char *name,
+const CompilerType _type, int storage) {
   ASTContext *ast = getASTContext();
   assert(ast != nullptr);
-  return ParmVarDecl::Create(*ast, ast->getTranslationUnitDecl(),
- SourceLocation(), SourceLocation(),
+  return ParmVarDecl::Create(*ast, decl_ctx, SourceLocation(), 
SourceLocation(),
  name && name[0] ? >Idents.get(name) : 
nullptr,
  ClangUtil::GetQualType(param_type), nullptr,
  (clang::StorageClass)storage, nullptr);
Index: lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
===
--- lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
+++ lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
@@ -932,7 +932,8 @@
 continue;
 
   clang::ParmVarDecl *param = m_ast.CreateParameterDeclaration(
-  nullptr, arg_type->GetForwardCompilerType(), clang::SC_None);
+  decl, nullptr, arg_type->GetForwardCompilerType(),
+  clang::SC_None);
   if (param)
 params.push_back(param);
 }
Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -635,8 +635,8 @@
 PdbCompilandSymId param_uid(func_id.modi, record_offset);
 TypeSP type_sp = GetOrCreateType(param_type);
 clang::ParmVarDecl *param = m_clang->CreateParameterDeclaration(
-param_name.str().c_str(), type_sp->GetForwardCompilerType(),
-clang::SC_None);
+function_decl, param_name.str().c_str(),
+type_sp->GetForwardCompilerType(), clang::SC_None);
 lldbassert(m_uid_to_decl.count(toOpaqueUid(param_uid)) == 0);
 
 m_uid_to_decl[toOpaqueUid(param_uid)] = param;
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -3407,8 +3407,9 @@
 function_param_types.push_back(type->GetForwardCompilerType());
 
 clang::ParmVarDecl *param_var_decl =
-m_ast.CreateParameterDeclaration(
-name, type->GetForwardCompilerType(), storage);
+m_ast.CreateParameterDeclaration(containing_decl_ctx, name,
+ 
type->GetForwardCompilerType(),
+ storage);
 assert(param_var_decl);
 function_param_decls.push_back(param_var_decl);
 
Index: lldb/include/lldb/Symbol/ClangASTContext.h
===
--- lldb/include/lldb/Symbol/ClangASTContext.h
+++ lldb/include/lldb/Symbol/ClangASTContext.h
@@ -401,7 +401,8 @@
type_quals, cc);
   }
 
-  clang::ParmVarDecl *CreateParameterDeclaration(const char *name,
+  clang::ParmVarDecl *CreateParameterDeclaration(clang::DeclContext *decl_ctx,
+ const char *name,
  const CompilerType 
_type,
  int storage);
 


Index: 

[Lldb-commits] [PATCH] D55569: [lit] Add a basic implementation of build for GccBuilder

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

@labath: Sorry, I didn't miss it, I just forgot about it (that's not much 
better, I know).  I'll look today.  If Pavel's is more complete let's use that 
as a starting point, although TBH I'm fine going with either one.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55569



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

[Lldb-commits] [PATCH] D55569: [lit] Add a basic implementation of build for GccBuilder

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Btw, I also wrote something like this in D55430 
. (Zachary asked me to do it, but it looks 
like he missed the review request. :P)

I think my version is more complete, though I am missing lto support, because I 
couldn't be bothered to figure out how it works. But if all that's needed is to 
add `-flto=thin`, then I can easily integrate that.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55569



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

[Lldb-commits] [PATCH] D55569: [lit] Add a basic implementation of build for GccBuilder

[Stella Stamenova via Phabricator via lldb-commits]

stella.stamenova created this revision.
stella.stamenova added reviewers: zturner, asmith.
Herald added subscribers: lldb-commits, abidh.

This adds an implementation for compile as well as compile-and-link, but not 
link. For compile-and-link we rely on clang to orchestrate both similarly to 
how the tests use it today.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D55569

Files:
  lit/BuildScript/modes-nonwin.test
  lit/BuildScript/modes-win.test
  lit/BuildScript/modes.test
  lit/BuildScript/script-args.test
  lit/helper/build.py

Index: lit/helper/build.py
===
--- lit/helper/build.py
+++ lit/helper/build.py
@@ -151,11 +151,14 @@
 return result
 
 def print_environment(env):
-for e in env:
-value = env[e]
-lines = value.split(os.pathsep)
-formatted_value = format_text(lines, 0, 7 + len(e))
-print('{0} = {1}'.format(e, formatted_value))
+if env:
+for e in env:
+value = env[e]
+lines = value.split(os.pathsep)
+formatted_value = format_text(lines, 0, 7 + len(e))
+print('{0} = {1}'.format(e, formatted_value))
+else:
+print('No environment specified')
 
 def find_executable(binary_name, search_paths):
 if sys.platform == 'win32':
@@ -604,11 +607,108 @@
 def __init__(self, toolchain_type, args):
 Builder.__init__(self, toolchain_type, args)
 
+def _output_name(self, input, extension, with_executable=False):
+basename = os.path.splitext(os.path.basename(input))[0] + extension
+if with_executable:
+exe_basename = os.path.basename(self._exe_file_name())
+basename = exe_basename + '-' + basename
+
+output = os.path.join(self.outdir, basename)
+return os.path.normpath(output)
+
+def _obj_file_names(self):
+if self.mode == 'link':
+return self.inputs
+
+if self.mode == 'compile-and-link':
+# Object file names should factor in both the input file (source)
+# name and output file (executable) name, to ensure that two tests
+# which share a common source file don't race to write the same
+# object file.
+return [self._output_name(x, '.o', True) for x in self.inputs]
+
+if self.mode == 'compile' and self.output:
+return [self.output]
+
+return [self._output_name(x, '.o') for x in self.inputs]
+
+def _exe_file_name(self):
+if self.mode == 'compile':
+return None
+return self.output
+
+def _get_compilation_command(self, source, obj):
+args = []
+
+args.append(self.compiler)
+
+if self.opt == 'none':
+args.append('-O0')
+elif self.opt == 'basic':
+args.append('-O2')
+elif self.opt == 'lto':
+args.append('-flto=thin')
+
+args.append('-c')
+args.append('-g')
+args.append('-o' + obj)
+args.append(source)
+
+return ('compiling', [source], obj,
+None,
+args)
+
+def _get_compile_and_link_command(self):
+args = []
+
+args.append(self.compiler)
+
+if self.opt == 'none':
+args.append('-O0')
+elif self.opt == 'basic':
+args.append('-O2')
+elif self.opt == 'lto':
+args.append('-flto=thin')
+
+args.append('-g')
+args.append('-o' + self._exe_file_name())
+args.extend(self.inputs)
+
+return ('compiling and linking', self.inputs, self._exe_file_name(),
+None,
+args)
+
+def _get_link_command(self):
+args = []
+args.append('echo linking only not supported')
+
+return ('linking', self._obj_file_names(), self._exe_file_name(),
+None,
+args)
+
 def build_commands(self):
-pass
+commands = []
+if self.mode == 'compile':
+for input, output in zip(self.inputs, self._obj_file_names()):
+commands.append(self._get_compilation_command(input, output))
+if self.mode == 'compile-and-link':
+commands.append(self._get_compile_and_link_command())
+if self.mode == 'link':
+commands.append(self._get_link_command())
+return commands
 
 def output_files(self):
-pass
+outdir = os.path.dirname(self.output)
+file = os.path.basename(self.output)
+name, ext = os.path.splitext(file)
+
+outputs = []
+if self.mode == 'compile' or self.mode == 'compile-and-link':
+outputs.extend(self._obj_file_names())
+if self.mode == 'link' or self.mode == 'compile-and-link':
+outputs.append(self._exe_file_name())
+
+return [x for x in outputs if x is not None]
 
 def indent(text, spaces):
 def prefixed_lines():
Index: 

[Lldb-commits] [lldb] r348894 - [Driver] Simplify OptionData. NFC

[Jonas Devlieghere via lldb-commits]

Author: jdevlieghere
Date: Tue Dec 11 12:19:53 2018
New Revision: 348894

URL: http://llvm.org/viewvc/llvm-project?rev=348894=rev
Log:
[Driver] Simplify OptionData. NFC

Hopefully this makes the option data easier to understand and maintain.

 - Group the member variables.
 - Do the initialization in the header as it's less error prone.
 - Rename the Clean method. It was called only once and was
   re-initializing some but not all (?) members. The only useful thing it
   does is dealing with the local lldbinit file so keep that and make the
   name reflect that.

Modified:
lldb/trunk/tools/driver/Driver.cpp
lldb/trunk/tools/driver/Driver.h

Modified: lldb/trunk/tools/driver/Driver.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/driver/Driver.cpp?rev=348894=348893=348894=diff
==
--- lldb/trunk/tools/driver/Driver.cpp (original)
+++ lldb/trunk/tools/driver/Driver.cpp Tue Dec 11 12:19:53 2018
@@ -120,53 +120,22 @@ Driver::Driver()
 
 Driver::~Driver() { g_driver = NULL; }
 
-Driver::OptionData::OptionData()
-: m_args(), m_script_lang(lldb::eScriptLanguageDefault), m_core_file(),
-  m_crash_log(), m_initial_commands(), m_after_file_commands(),
-  m_after_crash_commands(), m_debug_mode(false), m_source_quietly(false),
-  m_print_version(false), m_print_python_path(false), m_wait_for(false),
-  m_repl(false), m_repl_lang(eLanguageTypeUnknown), m_repl_options(),
-  m_process_name(), m_process_pid(LLDB_INVALID_PROCESS_ID),
-  m_use_external_editor(false), m_batch(false), m_seen_options() {}
-
-Driver::OptionData::~OptionData() {}
-
-void Driver::OptionData::Clear() {
-  m_args.clear();
-  m_script_lang = lldb::eScriptLanguageDefault;
-  m_initial_commands.clear();
-  m_after_file_commands.clear();
-
-  // If there is a local .lldbinit, add that to the
-  // list of things to be sourced, if the settings
-  // permit it.
+void Driver::OptionData::AddLocalLLDBInit() {
+  // If there is a local .lldbinit, add that to the list of things to be
+  // sourced, if the settings permit it.
   SBFileSpec local_lldbinit(".lldbinit", true);
-
   SBFileSpec homedir_dot_lldb = SBHostOS::GetUserHomeDirectory();
   homedir_dot_lldb.AppendPathComponent(".lldbinit");
 
-  // Only read .lldbinit in the current working directory
-  // if it's not the same as the .lldbinit in the home
-  // directory (which is already being read in).
+  // Only read .lldbinit in the current working directory if it's not the same
+  // as the .lldbinit in the home directory (which is already being read in).
   if (local_lldbinit.Exists() && strcmp(local_lldbinit.GetDirectory(),
 homedir_dot_lldb.GetDirectory()) != 0) 
{
-char path[2048];
-local_lldbinit.GetPath(path, 2047);
+char path[PATH_MAX];
+local_lldbinit.GetPath(path, sizeof(path));
 InitialCmdEntry entry(path, true, true, true);
 m_after_file_commands.push_back(entry);
   }
-
-  m_debug_mode = false;
-  m_source_quietly = false;
-  m_print_version = false;
-  m_print_python_path = false;
-  m_use_external_editor = false;
-  m_wait_for = false;
-  m_process_name.erase();
-  m_batch = false;
-  m_after_crash_commands.clear();
-
-  m_process_pid = LLDB_INVALID_PROCESS_ID;
 }
 
 void Driver::OptionData::AddInitialCommand(std::string command,
@@ -201,8 +170,6 @@ void Driver::OptionData::AddInitialComma
 command_set->push_back(InitialCmdEntry(command, is_file, false));
 }
 
-void Driver::ResetOptionValues() { m_option_data.Clear(); }
-
 const char *Driver::GetFilename() const {
   if (m_option_data.m_args.empty())
 return NULL;
@@ -284,7 +251,7 @@ bool Driver::GetDebugMode() const { retu
 // user only wanted help or version information.
 SBError Driver::ProcessArgs(const opt::InputArgList , bool ) {
   SBError error;
-  ResetOptionValues();
+  m_option_data.AddLocalLLDBInit();
 
   // This is kind of a pain, but since we make the debugger in the Driver's
   // constructor, we can't know at that point whether we should read in init

Modified: lldb/trunk/tools/driver/Driver.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/driver/Driver.h?rev=348894=348893=348894=diff
==
--- lldb/trunk/tools/driver/Driver.h (original)
+++ lldb/trunk/tools/driver/Driver.h Tue Dec 11 12:19:53 2018
@@ -57,13 +57,8 @@ public:
 
   bool GetDebugMode() const;
 
-  class OptionData {
-  public:
-OptionData();
-~OptionData();
-
-void Clear();
-
+  struct OptionData {
+void AddLocalLLDBInit();
 void AddInitialCommand(std::string command, CommandPlacement placement,
bool is_file, lldb::SBError );
 
@@ -71,36 +66,44 @@ public:
   InitialCmdEntry(std::string contents, bool in_is_file,
   bool is_cwd_lldbinit_file_read, bool in_quiet = false)
   : 

Re: [Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

[Jim Ingham via lldb-commits]

In MachO a segment fully contains its sections and a section can only be in one 
segment.  I don't remember how much the lldb Section nesting code enforces 
this.  It would be weird to have two Sections at the same level overlap, I 
don't see any "GetSectionsForAddress(lldb::addr_t address)" API's so I don't 
think we're ready to handle that.

Maybe we could start marking sections as "duplicate" and only handing out the 
original one when doing lldb::addr_t -> Section lookups?

Jim


> On Dec 11, 2018, at 11:58 AM, Pavel Labath  wrote:
> 
> On 11/12/2018 20:39, Jim Ingham wrote:
>> Sections can have parents.  In MachO the text and data sections are actually 
>> contained in the TEXT and DATA segments respectively.  LLDB represents this 
>> by having an lldb_private::Section for the segment, and then all the 
>> sections in that segment are children of the parent Section (the MachO 
>> segment).  All the code that looks up addresses expects to potentially 
>> traverse this hierarchy.
>> It seems to me that what you are describing should fit in the nesting model?
>> Jim
>>  
> 
> Hmm.. that's an interesting idea. I'll have to think about that.
> 
> Does MachO enforce somehow that every section is fully contained in one 
> segment? Because while that is true for elf in general, it is not a 
> requirement, and it is possible to create functional executables (though I 
> think I would have to use a hex editor for that) where one segment contains 
> only a half of some section. But that might not be a big issue, as I think 
> most tools will choke on files like this, so if we do something suboptimal 
> here, it shouldn't be too bad.
> 
> The other hickup I can think of is that sometimes (and this happens in 
> perfectly valid files too) one section is present in multiple segments. For 
> example .eh_frame will be present in a "LOAD" segment (so it ends up in 
> memory), but it will also be present in a special "GNU_EH_FRAME" segment (not 
> sure why, but I guess it's to make it easier for someone to find it). But I 
> think I can get around that by only creating these "container" sections for 
> LOAD segments.
> 
> 
> The part I know nothing about is whether something similar could be done for 
> PE/COFF files (and I'll need something like that there too). Adrian, Zachary, 
> what is the relation ship between "image base" of an object file and its 
> sections? Is there any way we could arrange so that the base address of a 
> module always belongs to one of its sections?
> 
> pl

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

Re: [Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

[Pavel Labath via lldb-commits]

On 11/12/2018 20:39, Jim Ingham wrote:

Sections can have parents.  In MachO the text and data sections are actually 
contained in the TEXT and DATA segments respectively.  LLDB represents this by 
having an lldb_private::Section for the segment, and then all the sections in 
that segment are children of the parent Section (the MachO segment).  All the 
code that looks up addresses expects to potentially traverse this hierarchy.

It seems to me that what you are describing should fit in the nesting model?

Jim
  


Hmm.. that's an interesting idea. I'll have to think about that.

Does MachO enforce somehow that every section is fully contained in one 
segment? Because while that is true for elf in general, it is not a 
requirement, and it is possible to create functional executables (though 
I think I would have to use a hex editor for that) where one segment 
contains only a half of some section. But that might not be a big issue, 
as I think most tools will choke on files like this, so if we do 
something suboptimal here, it shouldn't be too bad.


The other hickup I can think of is that sometimes (and this happens in 
perfectly valid files too) one section is present in multiple segments. 
For example .eh_frame will be present in a "LOAD" segment (so it ends up 
in memory), but it will also be present in a special "GNU_EH_FRAME" 
segment (not sure why, but I guess it's to make it easier for someone to 
find it). But I think I can get around that by only creating these 
"container" sections for LOAD segments.



The part I know nothing about is whether something similar could be done 
for PE/COFF files (and I'll need something like that there too). Adrian, 
Zachary, what is the relation ship between "image base" of an object 
file and its sections? Is there any way we could arrange so that the 
base address of a module always belongs to one of its sections?


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

Re: [Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

[Jim Ingham via lldb-commits]

Sections can have parents.  In MachO the text and data sections are actually 
contained in the TEXT and DATA segments respectively.  LLDB represents this by 
having an lldb_private::Section for the segment, and then all the sections in 
that segment are children of the parent Section (the MachO segment).  All the 
code that looks up addresses expects to potentially traverse this hierarchy.

It seems to me that what you are describing should fit in the nesting model?

Jim
 
> On Dec 11, 2018, at 11:31 AM, Pavel Labath  wrote:
> 
> On 11/12/2018 20:10, Jim Ingham via Phabricator wrote:
>> jingham added a comment.
>> In D55356#1327280 , @clayborg wrote:
>>> In D55356#1327242 , @labath wrote:
>>> 
 In D55356#1327224 , @clayborg 
 wrote:
 
> In D55356#1327099 , @labath 
> wrote:
> 
>> Actually, this now causes an lldb-mi test to fail, but it's not clear to 
>> me if the problem is in the test, or this patch. This issue happens when 
>> lldb-mi is printing the "library loaded" message after a module gets 
>> added to a not-yet-running target. It tries to print the load address by 
>> first getting the base address and then converting that to a load 
>> address.
>> 
>> Before this patch, that would always fail, because well.. ELF and PECOFF 
>> had this function unimplemented, and for MachO the base address was 
>> section-relative, and so it wasn't resolved to a load address without 
>> the section being loaded. However, with this patch, in the ELF (and 
>> presumably PECOFF) case, the load address is not section-relative and so 
>> the `GetLoadAddress` function happily returns the address.
>> 
>> Is this the expected behavior here? (i.e., 
>> object_file->GetLoadAddress().GetLoadAddress(target) returning a valid 
>> value even though the target is not running)
> 
> 
> Not unless someone has manually set the section load address in the test?
 
 
 The test is not setting the load address manually. This simply falls out 
 of how `Address::GetLoadAddress`  is implemented:
 
   addr_t Address::GetLoadAddress(Target *target) const {
 SectionSP section_sp(GetSection());
 if (section_sp) {
   ...
 } else {
   // We don't have a section so the offset is the load address
   return m_offset;
 }
   }
 
 
 So, where's the bug here? It's not clear to me how to change 
 `Address::GetLoadAddress` to do something different than what it is doing 
 now.
>>> 
>>> 
>>> So this is a bug really. When there is no section, we should specify what 
>>> the m_offset is using lldb_private::AddressType in maybe a new ivar name 
>>> "m_offset_type". Then GetBaseAddress() would specify eAddressTypeFile. And 
>>> the above code would become:
>>> 
>>>  
>>> addr_t Address::GetLoadAddress(Target *target) const {
>>> 
>>>   SectionSP section_sp(GetSection());
>>>   if (section_sp) {
>>> ...
>>>   } else if (m_offset_type == eAddressTypeLoad) {
>>> // We don't have a section so the offset is the load address
>>> return m_offset;
>>>   }
>>> 
>>> }
>>> 
>>>   We just need to be careful and see if we can not make 
>>> lldb_private::Address get any bigger byte size wise if we can at all avoid 
>>> it.
>> I must be missing something.  If there's a file around so that we can 
>> express this address relative to the file, why would it ever not be 
>> expressed as a section + offset?  If there's not a file around, then what 
>> does it mean to say this address ie eAddressTypeFile but we don't know the 
>> file?
> 
> I think the issue here is the difference in how elf and MachO files are 
> loaded. Elf has this strange dual view of the file, where the same data is 
> represented both as "sections", and "segments". Sections are the thing we all 
> know (.text, .data, .debug_info, etc.), and are used by most tools (lldb, 
> linker, ...). However, this is *not* what the kernel uses when it loads a 
> binary into memory. The loader uses "segments" instead.
> 
> It is segments who describe where will a piece of file be loaded into memory. 
> Normally, segments span one or more sections, but this is not a requirement. 
> And almost always segments will contain some additional data besides section 
> content. This data is generally "junk" but it is there because it enables the 
> linker to "pack" the elf file more efficiently.
> 
> A typical elf file might look something like
> ---
> | elf header  |
> ---
> | segment headers |
> ---
> | .text   |
> ---
> | other sections  |
> ---
> | section headers |
> ---
> 
> The segment headers might say "load everything from the start of 

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

[Zachary Turner via lldb-commits]

I meant the location of the minidump.  So if you have C:\A\B\C\foo.dmp
which is the dump file for bar.exe which crashed on another machine, then
it would look for C:\A\B\C\bar.pdb.  That actually seems like fairly
intuitive behavior to me, but maybe I'm in the minority :)

We can see what Pavel, Adrian, and others think though or if they have any
other suggestions.

On Tue, Dec 11, 2018 at 11:21 AM Leonard Mosescu  wrote:

> But if the minidump and PDBs are in the same directory, then wouldn't my
>> proposed solution also work (while also being a permanent solution)?
>>
>
> If we're looking in the same directory as the binary file (which is how I
> read your suggestion) then it would not be found in this case, since the
> binary path is coming from the machine where the crash occurred (so there's
> no relation with the host where we run LLDB). Using the minidump location
> as search path seems an even bigger hack than searching the current
> directory.
>
> Speaking of the current directory, I still don't like to use it implicitly
> in general, but it's actually consistent with what LLDB does for DWARF (see
> Symbols::LocateExecutableSymbolFile). So maybe it's not the terrible hack I
> made it look like.
>
> On Tue, Dec 11, 2018 at 10:48 AM Zachary Turner 
> wrote:
>
>> But if the minidump and PDBs are in the same directory, then wouldn't my
>> proposed solution also work (while also being a permanent solution)?
>>
>> On Tue, Dec 11, 2018 at 10:47 AM Leonard Mosescu 
>> wrote:
>>
>>> We talked about this offline, but bringing the discussion back here.
 Can you describe the use case that this is addressing?  As you mention,
 this is a temporary hack until we have proper symbol searching logic, but
 proper symbol searching logic will do more than just look up symbols in a
 symbol server.  It will also, for example, look in the same directory as
 the executable file.  If we changed this logic to do that, would your use
 case still be addressed?  At least that way, the logic we're adding is not
 temporary, even if it will eventually live in a different place (e.g. the
 SymbolVendor).

>>>
>>> This is intended to provide an easy way to experiment with minidumps +
>>> PDBs: just copy the minidump and the PDBs in the same directory (and run
>>> lldb from there).
>>>
>>> It's far from a general solution. I don't think that defaulting to the
>>> current directory should even be a hardcoded default - it's just a
>>> convenient but temporary hack. I'm open to any alternative ideas we can use
>>> until we implement a SymbolVendor.
>>>
>>> On Tue, Dec 11, 2018 at 10:39 AM Zachary Turner via Phabricator <
>>> revi...@reviews.llvm.org> wrote:
>>>
 zturner added inline comments.


 
 Comment at:
 source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:139-144
 +llvm::consumeError(expected_binary.takeError());
 +pdb_file = obj_file.GetFileSpec()
 +   .GetFileNameStrippingExtension()
 +   .GetStringRef()
 +   .str();
 +pdb_file += ".pdb";
 
 We talked about this offline, but bringing the discussion back here.
 Can you describe the use case that this is addressing?  As you mention,
 this is a temporary hack until we have proper symbol searching logic, but
 proper symbol searching logic will do more than just look up symbols in a
 symbol server.  It will also, for example, look in the same directory as
 the executable file.  If we changed this logic to do that, would your use
 case still be addressed?  At least that way, the logic we're adding is not
 temporary, even if it will eventually live in a different place (e.g. the
 SymbolVendor).


 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] D55356: Add a method to get the "base" file address of an object file

[Pavel Labath via lldb-commits]

On 11/12/2018 20:10, Jim Ingham via Phabricator wrote:

jingham added a comment.

In D55356#1327280 , @clayborg wrote:


In D55356#1327242 , @labath wrote:


In D55356#1327224 , @clayborg wrote:


In D55356#1327099 , @labath wrote:


Actually, this now causes an lldb-mi test to fail, but it's not clear to me if the 
problem is in the test, or this patch. This issue happens when lldb-mi is printing the 
"library loaded" message after a module gets added to a not-yet-running target. 
It tries to print the load address by first getting the base address and then converting 
that to a load address.

Before this patch, that would always fail, because well.. ELF and PECOFF had 
this function unimplemented, and for MachO the base address was 
section-relative, and so it wasn't resolved to a load address without the 
section being loaded. However, with this patch, in the ELF (and presumably 
PECOFF) case, the load address is not section-relative and so the 
`GetLoadAddress` function happily returns the address.

Is this the expected behavior here? (i.e., 
object_file->GetLoadAddress().GetLoadAddress(target) returning a valid value 
even though the target is not running)



Not unless someone has manually set the section load address in the test?



The test is not setting the load address manually. This simply falls out of how 
`Address::GetLoadAddress`  is implemented:

   addr_t Address::GetLoadAddress(Target *target) const {
 SectionSP section_sp(GetSection());
 if (section_sp) {
   ...
 } else {
   // We don't have a section so the offset is the load address
   return m_offset;
 }
   }


So, where's the bug here? It's not clear to me how to change 
`Address::GetLoadAddress` to do something different than what it is doing now.



So this is a bug really. When there is no section, we should specify what the m_offset is 
using lldb_private::AddressType in maybe a new ivar name "m_offset_type". Then 
GetBaseAddress() would specify eAddressTypeFile. And the above code would become:

  


addr_t Address::GetLoadAddress(Target *target) const {

   SectionSP section_sp(GetSection());
   if (section_sp) {
 ...
   } else if (m_offset_type == eAddressTypeLoad) {
 // We don't have a section so the offset is the load address
 return m_offset;
   }

}

   We just need to be careful and see if we can not make lldb_private::Address 
get any bigger byte size wise if we can at all avoid it.



I must be missing something.  If there's a file around so that we can express 
this address relative to the file, why would it ever not be expressed as a 
section + offset?  If there's not a file around, then what does it mean to say 
this address ie eAddressTypeFile but we don't know the file?




I think the issue here is the difference in how elf and MachO files are 
loaded. Elf has this strange dual view of the file, where the same data 
is represented both as "sections", and "segments". Sections are the 
thing we all know (.text, .data, .debug_info, etc.), and are used by 
most tools (lldb, linker, ...). However, this is *not* what the kernel 
uses when it loads a binary into memory. The loader uses "segments" instead.


It is segments who describe where will a piece of file be loaded into 
memory. Normally, segments span one or more sections, but this is not a 
requirement. And almost always segments will contain some additional 
data besides section content. This data is generally "junk" but it is 
there because it enables the linker to "pack" the elf file more efficiently.


A typical elf file might look something like
---
| elf header  |
---
| segment headers |
---
| .text   |
---
| other sections  |
---
| section headers |
---

The segment headers might say "load everything from the start of file to 
the end of .text section at address 0x4". So the load address of 
this file (and the base for all kinds of offsets) would be "0x4", 
but that does not correspond to any section (the file address of the 
.text section would probably be something like 0x40123).


So, it almost sounds to me like we would need some kind of a 
module-relative (in addition to section-relative) address. Then, this 
address would be represented as "module+0", and GetLoadAddress(target) 
could translate that the same way as it does section-relative addresses.


Though that may be overkill here. For my purposes it would be sufficient 
to just have a function which always returns an file address (regardless 
of whether it points to a section or not), and I can use it to compute 
offsets (kind of like my original patch did). We could keep the 
Module.GetBaseAddress returning LLDB_INVALID_ADDRESS for cases when the 
base address cannot be 

[Lldb-commits] [lldb] r348890 - Add ObjectFileBreakpad.{cpp, h} to the Xcode project.

[Jim Ingham via lldb-commits]

Author: jingham
Date: Tue Dec 11 11:25:03 2018
New Revision: 348890

URL: http://llvm.org/viewvc/llvm-project?rev=348890=rev
Log:
Add ObjectFileBreakpad.{cpp,h} to the Xcode project.

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

Modified: lldb/trunk/lldb.xcodeproj/project.pbxproj
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lldb.xcodeproj/project.pbxproj?rev=348890=348889=348890=diff
==
--- lldb/trunk/lldb.xcodeproj/project.pbxproj (original)
+++ lldb/trunk/lldb.xcodeproj/project.pbxproj Tue Dec 11 11:25:03 2018
@@ -525,6 +525,7 @@
2689009613353E4200698AC0 /* ObjectContainerBSDArchive.cpp in 
Sources */ = {isa = PBXBuildFile; fileRef = 26A3B4AC1181454800381BC2 /* 
ObjectContainerBSDArchive.cpp */; };
2689009713353E4200698AC0 /* ObjectContainerUniversalMachO.cpp 
in Sources */ = {isa = PBXBuildFile; fileRef = 260C898010F57C5600BB2B04 /* 
ObjectContainerUniversalMachO.cpp */; };
268900DC13353E6F00698AC0 /* ObjectFile.cpp in Sources */ = {isa 
= PBXBuildFile; fileRef = 26BC7F4C10F1BC1A00F91463 /* ObjectFile.cpp */; };
+   4C9BF11B21C0467700FA4036 /* ObjectFileBreakpad.cpp in Sources 
*/ = {isa = PBXBuildFile; fileRef = 4C9BF11921C0467700FA4036 /* 
ObjectFileBreakpad.cpp */; };
2689009913353E4200698AC0 /* ObjectFileELF.cpp in Sources */ = 
{isa = PBXBuildFile; fileRef = 260C898510F57C5600BB2B04 /* ObjectFileELF.cpp 
*/; };
26EFC4CD18CFAF0D00865D87 /* ObjectFileJIT.cpp in Sources */ = 
{isa = PBXBuildFile; fileRef = 26EFC4CA18CFAF0D00865D87 /* ObjectFileJIT.cpp 
*/; };
2689009A13353E4200698AC0 /* ObjectFileMachO.cpp in Sources */ = 
{isa = PBXBuildFile; fileRef = 260C898810F57C5600BB2B04 /* ObjectFileMachO.cpp 
*/; };
@@ -2293,6 +2294,8 @@
260C898110F57C5600BB2B04 /* ObjectContainerUniversalMachO.h */ 
= {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = 
sourcecode.c.h; path = ObjectContainerUniversalMachO.h; sourceTree = ""; 
};
26BC7F4C10F1BC1A00F91463 /* ObjectFile.cpp */ = {isa = 
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; 
name = ObjectFile.cpp; path = source/Symbol/ObjectFile.cpp; sourceTree = 
""; };
26BC7C5E10F1B6E900F91463 /* ObjectFile.h */ = {isa = 
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = 
ObjectFile.h; path = include/lldb/Symbol/ObjectFile.h; sourceTree = ""; 
};
+   4C9BF11921C0467700FA4036 /* ObjectFileBreakpad.cpp */ = {isa = 
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; 
path = ObjectFileBreakpad.cpp; sourceTree = ""; };
+   4C9BF11821C0467700FA4036 /* ObjectFileBreakpad.h */ = {isa = 
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = 
ObjectFileBreakpad.h; sourceTree = ""; };
260C898510F57C5600BB2B04 /* ObjectFileELF.cpp */ = {isa = 
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; 
path = ObjectFileELF.cpp; sourceTree = ""; };
260C898610F57C5600BB2B04 /* ObjectFileELF.h */ = {isa = 
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = 
ObjectFileELF.h; sourceTree = ""; };
26EFC4CA18CFAF0D00865D87 /* ObjectFileJIT.cpp */ = {isa = 
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; 
path = ObjectFileJIT.cpp; sourceTree = ""; };
@@ -3963,6 +3966,7 @@
260C898210F57C5600BB2B04 /* ObjectFile */ = {
isa = PBXGroup;
children = (
+   4C9BF11621C0467700FA4036 /* Breakpad */,
260C898310F57C5600BB2B04 /* ELF */,
26EFC4C718CFAF0D00865D87 /* JIT */,
260C898710F57C5600BB2B04 /* Mach-O */,
@@ -6153,6 +6157,15 @@
path = PPC64;
sourceTree = "";
};
+   4C9BF11621C0467700FA4036 /* Breakpad */ = {
+   isa = PBXGroup;
+   children = (
+   4C9BF11821C0467700FA4036 /* 
ObjectFileBreakpad.h */,
+   4C9BF11921C0467700FA4036 /* 
ObjectFileBreakpad.cpp */,
+   );
+   path = Breakpad;
+   sourceTree = "";
+   };
4CCA643A13B40B82003BDF98 /* LanguageRuntime */ = {
isa = PBXGroup;
children = (
@@ -7737,6 +7750,7 @@
files = (
33E5E8471A674FB60024ED68 /* StringConvert.cpp 
in Sources */,
2689FFDA13353D9D00698AC0 /* lldb.cpp in Sources 
*/,
+   4C9BF11B21C0467700FA4036 /* 

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

[Leonard Mosescu via lldb-commits]

>
> But if the minidump and PDBs are in the same directory, then wouldn't my
> proposed solution also work (while also being a permanent solution)?
>

If we're looking in the same directory as the binary file (which is how I
read your suggestion) then it would not be found in this case, since the
binary path is coming from the machine where the crash occurred (so there's
no relation with the host where we run LLDB). Using the minidump location
as search path seems an even bigger hack than searching the current
directory.

Speaking of the current directory, I still don't like to use it implicitly
in general, but it's actually consistent with what LLDB does for DWARF (see
Symbols::LocateExecutableSymbolFile). So maybe it's not the terrible hack I
made it look like.

On Tue, Dec 11, 2018 at 10:48 AM Zachary Turner  wrote:

> But if the minidump and PDBs are in the same directory, then wouldn't my
> proposed solution also work (while also being a permanent solution)?
>
> On Tue, Dec 11, 2018 at 10:47 AM Leonard Mosescu 
> wrote:
>
>> We talked about this offline, but bringing the discussion back here.  Can
>>> you describe the use case that this is addressing?  As you mention, this is
>>> a temporary hack until we have proper symbol searching logic, but proper
>>> symbol searching logic will do more than just look up symbols in a symbol
>>> server.  It will also, for example, look in the same directory as the
>>> executable file.  If we changed this logic to do that, would your use case
>>> still be addressed?  At least that way, the logic we're adding is not
>>> temporary, even if it will eventually live in a different place (e.g. the
>>> SymbolVendor).
>>>
>>
>> This is intended to provide an easy way to experiment with minidumps +
>> PDBs: just copy the minidump and the PDBs in the same directory (and run
>> lldb from there).
>>
>> It's far from a general solution. I don't think that defaulting to the
>> current directory should even be a hardcoded default - it's just a
>> convenient but temporary hack. I'm open to any alternative ideas we can use
>> until we implement a SymbolVendor.
>>
>> On Tue, Dec 11, 2018 at 10:39 AM Zachary Turner via Phabricator <
>> revi...@reviews.llvm.org> wrote:
>>
>>> zturner added inline comments.
>>>
>>>
>>> 
>>> Comment at:
>>> source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:139-144
>>> +llvm::consumeError(expected_binary.takeError());
>>> +pdb_file = obj_file.GetFileSpec()
>>> +   .GetFileNameStrippingExtension()
>>> +   .GetStringRef()
>>> +   .str();
>>> +pdb_file += ".pdb";
>>> 
>>> We talked about this offline, but bringing the discussion back here.
>>> Can you describe the use case that this is addressing?  As you mention,
>>> this is a temporary hack until we have proper symbol searching logic, but
>>> proper symbol searching logic will do more than just look up symbols in a
>>> symbol server.  It will also, for example, look in the same directory as
>>> the executable file.  If we changed this logic to do that, would your use
>>> case still be addressed?  At least that way, the logic we're adding is not
>>> temporary, even if it will eventually live in a different place (e.g. the
>>> SymbolVendor).
>>>
>>>
>>> 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] D55356: Add a method to get the "base" file address of an object file

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D55356#1327280 , @clayborg wrote:

> In D55356#1327242 , @labath wrote:
>
> > In D55356#1327224 , @clayborg 
> > wrote:
> >
> > > In D55356#1327099 , @labath 
> > > wrote:
> > >
> > > > Actually, this now causes an lldb-mi test to fail, but it's not clear 
> > > > to me if the problem is in the test, or this patch. This issue happens 
> > > > when lldb-mi is printing the "library loaded" message after a module 
> > > > gets added to a not-yet-running target. It tries to print the load 
> > > > address by first getting the base address and then converting that to a 
> > > > load address.
> > > >
> > > > Before this patch, that would always fail, because well.. ELF and 
> > > > PECOFF had this function unimplemented, and for MachO the base address 
> > > > was section-relative, and so it wasn't resolved to a load address 
> > > > without the section being loaded. However, with this patch, in the ELF 
> > > > (and presumably PECOFF) case, the load address is not section-relative 
> > > > and so the `GetLoadAddress` function happily returns the address.
> > > >
> > > > Is this the expected behavior here? (i.e., 
> > > > object_file->GetLoadAddress().GetLoadAddress(target) returning a valid 
> > > > value even though the target is not running)
> > >
> > >
> > > Not unless someone has manually set the section load address in the test?
> >
> >
> > The test is not setting the load address manually. This simply falls out of 
> > how `Address::GetLoadAddress`  is implemented:
> >
> >   addr_t Address::GetLoadAddress(Target *target) const {
> > SectionSP section_sp(GetSection());
> > if (section_sp) {
> >   ...
> > } else {
> >   // We don't have a section so the offset is the load address
> >   return m_offset;
> > }
> >   }
> >
> >
> > So, where's the bug here? It's not clear to me how to change 
> > `Address::GetLoadAddress` to do something different than what it is doing 
> > now.
>
>
> So this is a bug really. When there is no section, we should specify what the 
> m_offset is using lldb_private::AddressType in maybe a new ivar name 
> "m_offset_type". Then GetBaseAddress() would specify eAddressTypeFile. And 
> the above code would become:
>
>  
>
> addr_t Address::GetLoadAddress(Target *target) const {
>
>   SectionSP section_sp(GetSection());
>   if (section_sp) {
> ...
>   } else if (m_offset_type == eAddressTypeLoad) {
> // We don't have a section so the offset is the load address
> return m_offset;
>   }
>
> }
>
>   We just need to be careful and see if we can not make lldb_private::Address 
> get any bigger byte size wise if we can at all avoid it.


I must be missing something.  If there's a file around so that we can express 
this address relative to the file, why would it ever not be expressed as a 
section + offset?  If there's not a file around, then what does it mean to say 
this address ie eAddressTypeFile but we don't know the file?


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

https://reviews.llvm.org/D55356



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

Re: [Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

[Jim Ingham via lldb-commits]

The current behavior is definitely correct.  An Address without a section is 
generally going to be something like a stack or heap address.  Those definitely 
have load addresses of whatever their value is.

I'm not sure what it means to have a file address represented as an offset not 
a section with offset.  That might end up matching some address in an object 
file on load, but that's entirely an accident from lldb's perspective.  Can you 
not make this as a section relative thing when it is a file address?

Jim


> On Dec 11, 2018, at 10:42 AM, Pavel Labath  wrote:
> 
> On 11/12/2018 19:17, Jim Ingham wrote:
>> It the section isn't in the target's SectionLoadList, then GetLoadAddress 
>> should return LLDB_INVALID_ADDRESS.  I.e. this bit from 
>> Section::GetLoadBaseAddress:
>> load_base_addr = target->GetSectionLoadList().GetSectionLoadAddress(
>> const_cast(this)->shared_from_this());
>> should return LLDB_INVALID_ADDRESS because the section isn't in the load 
>> list.
>> Are we putting sections in the target's section load list before we've 
>> actually seen them loaded.  I'm pretty sure we shouldn't do that.
>> Jim
> 
> The issue here is that the Address object in question is not backed by any 
> section. It has a null section, and just an offset member.
> In this case Address::GetLoadAddress just decides to return that address as a 
> load address, which I imagine is correct in some situations, but in this case 
> our intention was for this address to represent a file address (which would 
> still need to be adjusted to account for where the module ended up being 
> loaded).
> 
> 

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

[Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D55356#1327242 , @labath wrote:

> In D55356#1327224 , @clayborg wrote:
>
> > In D55356#1327099 , @labath wrote:
> >
> > > Actually, this now causes an lldb-mi test to fail, but it's not clear to 
> > > me if the problem is in the test, or this patch. This issue happens when 
> > > lldb-mi is printing the "library loaded" message after a module gets 
> > > added to a not-yet-running target. It tries to print the load address by 
> > > first getting the base address and then converting that to a load address.
> > >
> > > Before this patch, that would always fail, because well.. ELF and PECOFF 
> > > had this function unimplemented, and for MachO the base address was 
> > > section-relative, and so it wasn't resolved to a load address without the 
> > > section being loaded. However, with this patch, in the ELF (and 
> > > presumably PECOFF) case, the load address is not section-relative and so 
> > > the `GetLoadAddress` function happily returns the address.
> > >
> > > Is this the expected behavior here? (i.e., 
> > > object_file->GetLoadAddress().GetLoadAddress(target) returning a valid 
> > > value even though the target is not running)
> >
> >
> > Not unless someone has manually set the section load address in the test?
>
>
> The test is not setting the load address manually. This simply falls out of 
> how `Address::GetLoadAddress`  is implemented:
>
>   addr_t Address::GetLoadAddress(Target *target) const {
> SectionSP section_sp(GetSection());
> if (section_sp) {
>   ...
> } else {
>   // We don't have a section so the offset is the load address
>   return m_offset;
> }
>   }
>
>
> So, where's the bug here? It's not clear to me how to change 
> `Address::GetLoadAddress` to do something different than what it is doing now.


So this is a bug really. When there is no section, we should specify what the 
m_offset is using lldb_private::AddressType in maybe a new ivar name 
"m_offset_type". Then GetBaseAddress() would specify eAddressTypeFile. And the 
above code would become:

  addr_t Address::GetLoadAddress(Target *target) const {
SectionSP section_sp(GetSection());
if (section_sp) {
  ...
} else if (m_offset_type == eAddressTypeLoad) {
  // We don't have a section so the offset is the load address
  return m_offset;
}
  }

We just need to be careful and see if we can not make lldb_private::Address get 
any bigger byte size wise if we can at all avoid it.


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

https://reviews.llvm.org/D55356



___
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]

But if the minidump and PDBs are in the same directory, then wouldn't my
proposed solution also work (while also being a permanent solution)?

On Tue, Dec 11, 2018 at 10:47 AM Leonard Mosescu  wrote:

> We talked about this offline, but bringing the discussion back here.  Can
>> you describe the use case that this is addressing?  As you mention, this is
>> a temporary hack until we have proper symbol searching logic, but proper
>> symbol searching logic will do more than just look up symbols in a symbol
>> server.  It will also, for example, look in the same directory as the
>> executable file.  If we changed this logic to do that, would your use case
>> still be addressed?  At least that way, the logic we're adding is not
>> temporary, even if it will eventually live in a different place (e.g. the
>> SymbolVendor).
>>
>
> This is intended to provide an easy way to experiment with minidumps +
> PDBs: just copy the minidump and the PDBs in the same directory (and run
> lldb from there).
>
> It's far from a general solution. I don't think that defaulting to the
> current directory should even be a hardcoded default - it's just a
> convenient but temporary hack. I'm open to any alternative ideas we can use
> until we implement a SymbolVendor.
>
> On Tue, Dec 11, 2018 at 10:39 AM Zachary Turner via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> zturner added inline comments.
>>
>>
>> 
>> Comment at:
>> source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:139-144
>> +llvm::consumeError(expected_binary.takeError());
>> +pdb_file = obj_file.GetFileSpec()
>> +   .GetFileNameStrippingExtension()
>> +   .GetStringRef()
>> +   .str();
>> +pdb_file += ".pdb";
>> 
>> We talked about this offline, but bringing the discussion back here.  Can
>> you describe the use case that this is addressing?  As you mention, this is
>> a temporary hack until we have proper symbol searching logic, but proper
>> symbol searching logic will do more than just look up symbols in a symbol
>> server.  It will also, for example, look in the same directory as the
>> executable file.  If we changed this logic to do that, would your use case
>> still be addressed?  At least that way, the logic we're adding is not
>> temporary, even if it will eventually live in a different place (e.g. the
>> SymbolVendor).
>>
>>
>> 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

[Leonard Mosescu via lldb-commits]

>
> We talked about this offline, but bringing the discussion back here.  Can
> you describe the use case that this is addressing?  As you mention, this is
> a temporary hack until we have proper symbol searching logic, but proper
> symbol searching logic will do more than just look up symbols in a symbol
> server.  It will also, for example, look in the same directory as the
> executable file.  If we changed this logic to do that, would your use case
> still be addressed?  At least that way, the logic we're adding is not
> temporary, even if it will eventually live in a different place (e.g. the
> SymbolVendor).
>

This is intended to provide an easy way to experiment with minidumps +
PDBs: just copy the minidump and the PDBs in the same directory (and run
lldb from there).

It's far from a general solution. I don't think that defaulting to the
current directory should even be a hardcoded default - it's just a
convenient but temporary hack. I'm open to any alternative ideas we can use
until we implement a SymbolVendor.

On Tue, Dec 11, 2018 at 10:39 AM Zachary Turner via Phabricator <
revi...@reviews.llvm.org> wrote:

> zturner added inline comments.
>
>
> 
> Comment at:
> source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:139-144
> +llvm::consumeError(expected_binary.takeError());
> +pdb_file = obj_file.GetFileSpec()
> +   .GetFileNameStrippingExtension()
> +   .GetStringRef()
> +   .str();
> +pdb_file += ".pdb";
> 
> We talked about this offline, but bringing the discussion back here.  Can
> you describe the use case that this is addressing?  As you mention, this is
> a temporary hack until we have proper symbol searching logic, but proper
> symbol searching logic will do more than just look up symbols in a symbol
> server.  It will also, for example, look in the same directory as the
> executable file.  If we changed this logic to do that, would your use case
> still be addressed?  At least that way, the logic we're adding is not
> temporary, even if it will eventually live in a different place (e.g. the
> SymbolVendor).
>
>
> 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] D55356: Add a method to get the "base" file address of an object file

[Pavel Labath via lldb-commits]

On 11/12/2018 19:17, Jim Ingham wrote:

It the section isn't in the target's SectionLoadList, then GetLoadAddress 
should return LLDB_INVALID_ADDRESS.  I.e. this bit from 
Section::GetLoadBaseAddress:

 load_base_addr = target->GetSectionLoadList().GetSectionLoadAddress(
 const_cast(this)->shared_from_this());

should return LLDB_INVALID_ADDRESS because the section isn't in the load list.

Are we putting sections in the target's section load list before we've actually 
seen them loaded.  I'm pretty sure we shouldn't do that.

Jim




The issue here is that the Address object in question is not backed by 
any section. It has a null section, and just an offset member.
In this case Address::GetLoadAddress just decides to return that address 
as a load address, which I imagine is correct in some situations, but in 
this case our intention was for this address to represent a file address 
(which would still need to be adjusted to account for where the module 
ended up being loaded).



___
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

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.



Comment at: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:139-144
+llvm::consumeError(expected_binary.takeError());
+pdb_file = obj_file.GetFileSpec()
+   .GetFileNameStrippingExtension()
+   .GetStringRef()
+   .str();
+pdb_file += ".pdb";

We talked about this offline, but bringing the discussion back here.  Can you 
describe the use case that this is addressing?  As you mention, this is a 
temporary hack until we have proper symbol searching logic, but proper symbol 
searching logic will do more than just look up symbols in a symbol server.  It 
will also, for example, look in the same directory as the executable file.  If 
we changed this logic to do that, would your use case still be addressed?  At 
least that way, the logic we're adding is not temporary, even if it will 
eventually live in a different place (e.g. the SymbolVendor).


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] D55356: Add a method to get the "base" file address of an object file

[Jim Ingham via lldb-commits]

It the section isn't in the target's SectionLoadList, then GetLoadAddress 
should return LLDB_INVALID_ADDRESS.  I.e. this bit from 
Section::GetLoadBaseAddress:

load_base_addr = target->GetSectionLoadList().GetSectionLoadAddress(
const_cast(this)->shared_from_this());

should return LLDB_INVALID_ADDRESS because the section isn't in the load list.

Are we putting sections in the target's section load list before we've actually 
seen them loaded.  I'm pretty sure we shouldn't do that.

Jim


> On Dec 11, 2018, at 9:58 AM, Pavel Labath via Phabricator via lldb-commits 
>  wrote:
> 
> labath added a comment.
> 
> In D55356#1327224 , @clayborg wrote:
> 
>> In D55356#1327099 , @labath wrote:
>> 
>>> Actually, this now causes an lldb-mi test to fail, but it's not clear to me 
>>> if the problem is in the test, or this patch. This issue happens when 
>>> lldb-mi is printing the "library loaded" message after a module gets added 
>>> to a not-yet-running target. It tries to print the load address by first 
>>> getting the base address and then converting that to a load address.
>>> 
>>> Before this patch, that would always fail, because well.. ELF and PECOFF 
>>> had this function unimplemented, and for MachO the base address was 
>>> section-relative, and so it wasn't resolved to a load address without the 
>>> section being loaded. However, with this patch, in the ELF (and presumably 
>>> PECOFF) case, the load address is not section-relative and so the 
>>> `GetLoadAddress` function happily returns the address.
>>> 
>>> Is this the expected behavior here? (i.e., 
>>> object_file->GetLoadAddress().GetLoadAddress(target) returning a valid 
>>> value even though the target is not running)
>> 
>> 
>> Not unless someone has manually set the section load address in the test?
> 
> 
> The test is not setting the load address manually. This simply falls out of 
> how `Address::GetLoadAddress`  is implemented:
> 
>  addr_t Address::GetLoadAddress(Target *target) const {
>SectionSP section_sp(GetSection());
>if (section_sp) {
>  ...
>} else {
>  // We don't have a section so the offset is the load address
>  return m_offset;
>}
>  }
> 
> So, where's the bug here? It's not clear to me how to change 
> `Address::GetLoadAddress` to do something different than what it is doing now.
> 
> 
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D55356/new/
> 
> https://reviews.llvm.org/D55356
> 
> 
> 
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D55356#1327224 , @clayborg wrote:

> In D55356#1327099 , @labath wrote:
>
> > Actually, this now causes an lldb-mi test to fail, but it's not clear to me 
> > if the problem is in the test, or this patch. This issue happens when 
> > lldb-mi is printing the "library loaded" message after a module gets added 
> > to a not-yet-running target. It tries to print the load address by first 
> > getting the base address and then converting that to a load address.
> >
> > Before this patch, that would always fail, because well.. ELF and PECOFF 
> > had this function unimplemented, and for MachO the base address was 
> > section-relative, and so it wasn't resolved to a load address without the 
> > section being loaded. However, with this patch, in the ELF (and presumably 
> > PECOFF) case, the load address is not section-relative and so the 
> > `GetLoadAddress` function happily returns the address.
> >
> > Is this the expected behavior here? (i.e., 
> > object_file->GetLoadAddress().GetLoadAddress(target) returning a valid 
> > value even though the target is not running)
>
>
> Not unless someone has manually set the section load address in the test?


The test is not setting the load address manually. This simply falls out of how 
`Address::GetLoadAddress`  is implemented:

  addr_t Address::GetLoadAddress(Target *target) const {
SectionSP section_sp(GetSection());
if (section_sp) {
  ...
} else {
  // We don't have a section so the offset is the load address
  return m_offset;
}
  }

So, where's the bug here? It's not clear to me how to change 
`Address::GetLoadAddress` to do something different than what it is doing now.


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

https://reviews.llvm.org/D55356



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

[Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D55356#1327224 , @clayborg wrote:

> In D55356#1327099 , @labath wrote:
>
> > Actually, this now causes an lldb-mi test to fail, but it's not clear to me 
> > if the problem is in the test, or this patch. This issue happens when 
> > lldb-mi is printing the "library loaded" message after a module gets added 
> > to a not-yet-running target. It tries to print the load address by first 
> > getting the base address and then converting that to a load address.
> >
> > Before this patch, that would always fail, because well.. ELF and PECOFF 
> > had this function unimplemented, and for MachO the base address was 
> > section-relative, and so it wasn't resolved to a load address without the 
> > section being loaded. However, with this patch, in the ELF (and presumably 
> > PECOFF) case, the load address is not section-relative and so the 
> > `GetLoadAddress` function happily returns the address.
> >
> > Is this the expected behavior here? (i.e., 
> > object_file->GetLoadAddress().GetLoadAddress(target) returning a valid 
> > value even though the target is not running)
>
>
> Not unless someone has manually set the section load address in the test?


But even if the test is setting the section load address, this won't work for 
anything else other than Darwin, so the test would need to be fixed,


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

https://reviews.llvm.org/D55356



___
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 Pavel and Greg.

It sounds to me like it would be better to have a separate command
> (let's call it "target modules replace" for now) for adding an "object
> file" to a "placeholder" module, instead of repurposing "target symbols
> add" to do that.


Yes, that would be my preference as well.

Imagine the following scenario:
> - I load a minidump, without any supporting files around ...


I'd like to support the "other" variation as well: we start with a
minidump, load the symbols, _then_ dig for some module's binary (for
example if we really need image bits that are not captured in the minidump)

On Tue, Dec 11, 2018 at 9:50 AM Greg Clayton  wrote:

>
>
> > On Dec 11, 2018, at 7:14 AM, Pavel Labath  wrote:
> >
> > On 11/12/2018 01:08, Greg Clayton wrote:
> >>> On Dec 10, 2018, at 3:11 PM, Leonard Mosescu  > wrote:
> >>>
> >>> I can see how this works for the PDB, no-module-binary case. What
> about the PDB & module-binary case?
> >> That would work fine because the symbol vendor will make an object file
> from the m_symfile_spec and use both the original binary + the symbol file
> to make symbols.
> >>> Maybe I missed the rationale, but I think that modeling the general
> case: module and symbols are separate files, is a better foundation for the
> particular case where the symbols are embedded in the binary file.
> >> Yeah, my case should handle the following cases:
> >> - Minidumps Placeholder modules only, no real binaries, add symbols by
> downloading them and using "target symbols add ..."
> >> - Download real binaries up front and load them into LLDB, then load
> the core file. For any binaries we do have, we should grab them from the
> global module cache (GetTarget().GetSharedModule(...) in the current code)
> and they will use real object files, not placeholder files. "target symbols
> add ..." still works in this case
> >
> > It sounds to me like it would be better to have a separate command
> (let's call it "target modules replace" for now) for adding an "object
> file" to a "placeholder" module, instead of repurposing "target symbols
> add" to do that.
> >
> > This creates a strange loop between the symbol and object files --
> normally we use the object file for symbol representation if the user
> hasn't specified a symbol file, and here, we would do the opposite.
> >
> > With "target modules replace", one command would still be enough to set
> both the symbol and object files if they are the same, as the default
> use-symbols-from-object-file behavior would kick in. However, it would
> leave the "target symbols add" command free to add symbols from an external
> file.
> >
> > Imagine the following scenario:
> > - I load a minidump, without any supporting files around
> > - I see that my object file is missing, I do some digging around, and
> manage to find the stripped object file for this module
> > - I do "target modules replace index_of_placeholder_module
> /path/to/elf.stripped"
> > - now my sections are there, but I still don't have symbols
> > - I do more digging and find the breakpad file
> > - I do "target symbols add /path/to/breakpad.syms"
> > - success. I now have both symbols and sections
> >
> > Now this is probably not the most common scenario, but I think it can be
> used as a benchmark to see if the infrastructure is set up the right way.
> If things are set up correctly this should work out-of-the-box. With your
> setup, the scenario above might "just work" if I execute "target symbols
> add" twice (with different files), because the second "target symbols add"
> will do something different than the first one, but that sounds to me like
> another reason why these should be different commands.
> >
> > Maybe there should be there should be some code to help the user and
> detect the situation when he is adding a symbol file to a placeholder
> module and the symbol file is able serve as a good object file too, but
> that code could live higher up than Module::GetObjectFile. It could perhaps
> be directly in the "target symbols add" command via something like:
> > if (module_I_am_about_to_add_symbols_to->IsPlaceholder() &&
> symbol_file->ContainsGoodObjectFileRepresentation())
> >  TargetModulesReplace(module_I_am_about_to_add_symbols_to, symbol_file)
> > else
> >  the_module_I_am_about_to_add_symbols_to->SetSymbolFile(symbol_file)
>
> I like the replace idea. Another scenario this will fix is when you start
> debugging with a stripped object file straight from a device, and later
> download the unstripped version and want to replace it. We will need to
> make sure all breakpoints get unresolved and re-resolved correctly in
> testing. Also, stack frames should get flushed as they do when we do
> "target symbols add"
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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 added a comment.

In D55522#1326955 , @labath wrote:

> It just occurred to me that we have another copy of /proc//maps -> 
> MemoryRegionInfo conversion code. It lives in NativeProcessLinux.cpp 
> (ParseMemoryRegionInfoFromProcMapsLine). It would be nice to extract this 
> parser into a some place (`Plugins/Process/Utility` ?), where it can be 
> shared by the two process plugins. As a bonus, that should make it easy to 
> write unit tests for that function.


Nice! I should have looked around. I will move it to Plugins/Process/Utility as 
suggested


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

https://reviews.llvm.org/D55522



___
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

[Greg Clayton via lldb-commits]

> On Dec 11, 2018, at 7:14 AM, Pavel Labath  wrote:
> 
> On 11/12/2018 01:08, Greg Clayton wrote:
>>> On Dec 10, 2018, at 3:11 PM, Leonard Mosescu >> > wrote:
>>> 
>>> I can see how this works for the PDB, no-module-binary case. What about the 
>>> PDB & module-binary case?
>> That would work fine because the symbol vendor will make an object file from 
>> the m_symfile_spec and use both the original binary + the symbol file to 
>> make symbols.
>>> Maybe I missed the rationale, but I think that modeling the general case: 
>>> module and symbols are separate files, is a better foundation for the 
>>> particular case where the symbols are embedded in the binary file.
>> Yeah, my case should handle the following cases:
>> - Minidumps Placeholder modules only, no real binaries, add symbols by 
>> downloading them and using "target symbols add ..."
>> - Download real binaries up front and load them into LLDB, then load the 
>> core file. For any binaries we do have, we should grab them from the global 
>> module cache (GetTarget().GetSharedModule(...) in the current code) and they 
>> will use real object files, not placeholder files. "target symbols add ..." 
>> still works in this case
> 
> It sounds to me like it would be better to have a separate command (let's 
> call it "target modules replace" for now) for adding an "object file" to a 
> "placeholder" module, instead of repurposing "target symbols add" to do that.
> 
> This creates a strange loop between the symbol and object files -- normally 
> we use the object file for symbol representation if the user hasn't specified 
> a symbol file, and here, we would do the opposite.
> 
> With "target modules replace", one command would still be enough to set both 
> the symbol and object files if they are the same, as the default 
> use-symbols-from-object-file behavior would kick in. However, it would leave 
> the "target symbols add" command free to add symbols from an external file.
> 
> Imagine the following scenario:
> - I load a minidump, without any supporting files around
> - I see that my object file is missing, I do some digging around, and manage 
> to find the stripped object file for this module
> - I do "target modules replace index_of_placeholder_module 
> /path/to/elf.stripped"
> - now my sections are there, but I still don't have symbols
> - I do more digging and find the breakpad file
> - I do "target symbols add /path/to/breakpad.syms"
> - success. I now have both symbols and sections
> 
> Now this is probably not the most common scenario, but I think it can be used 
> as a benchmark to see if the infrastructure is set up the right way. If 
> things are set up correctly this should work out-of-the-box. With your setup, 
> the scenario above might "just work" if I execute "target symbols add" twice 
> (with different files), because the second "target symbols add" will do 
> something different than the first one, but that sounds to me like another 
> reason why these should be different commands.
> 
> Maybe there should be there should be some code to help the user and detect 
> the situation when he is adding a symbol file to a placeholder module and the 
> symbol file is able serve as a good object file too, but that code could live 
> higher up than Module::GetObjectFile. It could perhaps be directly in the 
> "target symbols add" command via something like:
> if (module_I_am_about_to_add_symbols_to->IsPlaceholder() && 
> symbol_file->ContainsGoodObjectFileRepresentation())
>  TargetModulesReplace(module_I_am_about_to_add_symbols_to, symbol_file)
> else
>  the_module_I_am_about_to_add_symbols_to->SetSymbolFile(symbol_file)

I like the replace idea. Another scenario this will fix is when you start 
debugging with a stripped object file straight from a device, and later 
download the unstripped version and want to replace it. We will need to make 
sure all breakpoints get unresolved and re-resolved correctly in testing. Also, 
stack frames should get flushed as they do when we do "target symbols add"

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

[Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D55356#1327099 , @labath wrote:

> Actually, this now causes an lldb-mi test to fail, but it's not clear to me 
> if the problem is in the test, or this patch. This issue happens when lldb-mi 
> is printing the "library loaded" message after a module gets added to a 
> not-yet-running target. It tries to print the load address by first getting 
> the base address and then converting that to a load address.
>
> Before this patch, that would always fail, because well.. ELF and PECOFF had 
> this function unimplemented, and for MachO the base address was 
> section-relative, and so it wasn't resolved to a load address without the 
> section being loaded. However, with this patch, in the ELF (and presumably 
> PECOFF) case, the load address is not section-relative and so the 
> `GetLoadAddress` function happily returns the address.
>
> Is this the expected behavior here? (i.e., 
> object_file->GetLoadAddress().GetLoadAddress(target) returning a valid value 
> even though the target is not running)


Not unless someone has manually set the section load address in the test?


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

https://reviews.llvm.org/D55356



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

[Lldb-commits] [PATCH] D53677: Fix a bug PlatformDarwin::SDKSupportsModule

[Adrian Prantl via Phabricator via lldb-commits]

aprantl closed this revision.
aprantl added a comment.

Looks like this landed in r345274 and wasn't properly closed.


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

https://reviews.llvm.org/D53677



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

[Lldb-commits] [PATCH] D55559: Remove the Disassembly benchmarks.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl created this revision.
aprantl added reviewers: jingham, jasonmolenda.

While I was out hunting for remaining pexpect-based tests, I came across these 
tests that can't possibly work an any modern system, as they rely on having gdb 
available in /Developer.

This patch simply removes the test without replacement.


https://reviews.llvm.org/D9

Files:
  packages/Python/lldbsuite/test/benchmarks/disassembly/TestDisassembly.py
  
packages/Python/lldbsuite/test/benchmarks/disassembly/TestDoAttachThenDisassembly.py
  
packages/Python/lldbsuite/test/benchmarks/disassembly/TestXcode41Vs42GDBDisassembly.py

Index: packages/Python/lldbsuite/test/benchmarks/disassembly/TestXcode41Vs42GDBDisassembly.py
===
--- packages/Python/lldbsuite/test/benchmarks/disassembly/TestXcode41Vs42GDBDisassembly.py
+++ /dev/null
@@ -1,120 +0,0 @@
-"""Disassemble lldb's Driver::MainLoop() functions comparing Xcode 4.1 vs. 4.2's gdb."""
-
-from __future__ import print_function
-
-
-import os
-import sys
-import lldb
-from lldbsuite.test.decorators import *
-from lldbsuite.test.lldbbench import *
-from lldbsuite.test.lldbtest import *
-from lldbsuite.test import configuration
-from lldbsuite.test import lldbutil
-
-
-class XCode41Vs42GDBDisassembly(BenchBase):
-
-mydir = TestBase.compute_mydir(__file__)
-
-def setUp(self):
-BenchBase.setUp(self)
-self.gdb_41_exe = '/Xcode41/usr/bin/gdb'
-self.gdb_42_exe = '/Developer/usr/bin/gdb'
-self.exe = lldbtest_config.lldbExec
-self.function = 'Driver::MainLoop()'
-self.gdb_41_avg = None
-self.gdb_42_avg = None
-self.count = 5
-
-@benchmarks_test
-@no_debug_info_test
-@expectedFailureAll(
-oslist=["windows"],
-bugnumber="llvm.org/pr22274: need a pexpect replacement for windows")
-def test_run_41_then_42(self):
-"""Test disassembly on a large function with 4.1 vs. 4.2's gdb."""
-print()
-self.run_gdb_disassembly(
-self.gdb_41_exe,
-self.exe,
-self.function,
-self.count)
-print("4.1 gdb benchmark:", self.stopwatch)
-self.gdb_41_avg = self.stopwatch.avg()
-self.run_gdb_disassembly(
-self.gdb_42_exe,
-self.exe,
-self.function,
-self.count)
-print("4.2 gdb benchmark:", self.stopwatch)
-self.gdb_42_avg = self.stopwatch.avg()
-print("gdb_42_avg/gdb_41_avg: %f" %
-  (self.gdb_42_avg / self.gdb_41_avg))
-
-@benchmarks_test
-@no_debug_info_test
-@expectedFailureAll(
-oslist=["windows"],
-bugnumber="llvm.org/pr22274: need a pexpect replacement for windows")
-def test_run_42_then_41(self):
-"""Test disassembly on a large function with 4.1 vs. 4.2's gdb."""
-print()
-self.run_gdb_disassembly(
-self.gdb_42_exe,
-self.exe,
-self.function,
-self.count)
-print("4.2 gdb benchmark:", self.stopwatch)
-self.gdb_42_avg = self.stopwatch.avg()
-self.run_gdb_disassembly(
-self.gdb_41_exe,
-self.exe,
-self.function,
-self.count)
-print("4.1 gdb benchmark:", self.stopwatch)
-self.gdb_41_avg = self.stopwatch.avg()
-print("gdb_42_avg/gdb_41_avg: %f" %
-  (self.gdb_42_avg / self.gdb_41_avg))
-
-def run_gdb_disassembly(self, gdb_exe_path, exe, function, count):
-import pexpect
-# Set self.child_prompt, which is "(gdb) ".
-self.child_prompt = '(gdb) '
-prompt = self.child_prompt
-
-# So that the child gets torn down after the test.
-self.child = pexpect.spawn('%s --nx %s' % (gdb_exe_path, exe))
-child = self.child
-
-# Turn on logging for what the child sends back.
-if self.TraceOn():
-child.logfile_read = sys.stdout
-
-child.expect_exact(prompt)
-child.sendline('break %s' % function)
-child.expect_exact(prompt)
-child.sendline('run')
-child.expect_exact(prompt)
-
-# Reset the stopwatch now.
-self.stopwatch.reset()
-for i in range(count):
-with self.stopwatch:
-# Disassemble the function.
-child.sendline('disassemble')
-child.expect_exact(prompt)
-child.sendline('next')
-child.expect_exact(prompt)
-
-child.sendline('quit')
-child.expect_exact('The program is running.  Exit anyway?')
-child.sendline('y')
-try:
-self.child.expect(pexpect.EOF)
-except:
-pass
-
-if self.TraceOn():
-print("gdb disassembly benchmark:", str(self.stopwatch))
-self.child = None
Index: packages/Python/lldbsuite/test/benchmarks/disassembly/TestDoAttachThenDisassembly.py

[Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Actually, this now causes an lldb-mi test to fail, but it's not clear to me if 
the problem is in the test, or this patch. This issue happens when lldb-mi is 
printing the "library loaded" message after a module gets added to a 
not-yet-running target. It tries to print the load address by first getting the 
base address and then converting that to a load address.

Before this patch, that would always fail, because well.. ELF and PECOFF had 
this function unimplemented, and for MachO the base address was 
section-relative, and so it wasn't resolved to a load address without the 
section being loaded. However, with this patch, in the ELF (and presumably 
PECOFF) case, the load address is not section-relative and so the 
`GetLoadAddress` function happily returns the address.

Is this the expected behavior here? (i.e., 
object_file->GetLoadAddress().GetLoadAddress(target) returning a valid value 
even though the target is not running)


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

https://reviews.llvm.org/D55356



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

[Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

[Pavel Labath via Phabricator via lldb-commits]

labath updated this revision to Diff 177717.
labath added a comment.

Rebase after comitting D55422 . Now this patch 
just implements GetBaseAddress for
ELF and PECOFF object files, and adds tests.


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

https://reviews.llvm.org/D55356

Files:
  lit/Modules/ELF/basic-info.yaml
  lit/Modules/MachO/basic-info.yaml
  lit/Modules/PECOFF/basic-info.yaml
  lit/Modules/build-id-case.yaml
  lit/Modules/compressed-sections.yaml
  lit/Modules/elf-section-types.yaml
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.h
  source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
  tools/lldb-test/lldb-test.cpp

Index: tools/lldb-test/lldb-test.cpp
===
--- tools/lldb-test/lldb-test.cpp
+++ tools/lldb-test/lldb-test.cpp
@@ -752,6 +752,8 @@
 Printer.formatLine("Stripped: {0}", ObjectPtr->IsStripped());
 Printer.formatLine("Type: {0}", ObjectPtr->GetType());
 Printer.formatLine("Strata: {0}", ObjectPtr->GetStrata());
+Printer.formatLine("Base VM address: {0:x}",
+   ObjectPtr->GetBaseAddress().GetFileAddress());
 
 size_t Count = Sections->GetNumSections(0);
 Printer.formatLine("Showing {0} sections", Count);
@@ -762,6 +764,7 @@
   Printer.formatLine("Index: {0}", I);
   Printer.formatLine("Name: {0}", S->GetName().GetStringRef());
   Printer.formatLine("Type: {0}", S->GetTypeAsCString());
+  Printer.formatLine("VM address: {0:x}", S->GetFileAddress());
   Printer.formatLine("VM size: {0}", S->GetByteSize());
   Printer.formatLine("File size: {0}", S->GetFileSize());
 
Index: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
===
--- source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
+++ source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
@@ -118,6 +118,8 @@
 
   virtual lldb_private::Address GetEntryPointAddress() override;
 
+  lldb_private::Address GetBaseAddress() override { return m_image_base; }
+
   ObjectFile::Type CalculateType() override;
 
   ObjectFile::Strata CalculateStrata() override;
Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.h
===
--- source/Plugins/ObjectFile/ELF/ObjectFileELF.h
+++ source/Plugins/ObjectFile/ELF/ObjectFileELF.h
@@ -134,6 +134,8 @@
 
   lldb_private::Address GetEntryPointAddress() override;
 
+  lldb_private::Address GetBaseAddress() override;
+
   ObjectFile::Type CalculateType() override;
 
   ObjectFile::Strata CalculateStrata() override;
Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -805,21 +805,10 @@
 SectionList *section_list = GetSectionList();
 if (section_list) {
   if (!value_is_offset) {
-bool found_offset = false;
-for (size_t i = 1, count = GetProgramHeaderCount(); i <= count; ++i) {
-  const elf::ELFProgramHeader *header = GetProgramHeaderByIndex(i);
-  if (header == nullptr)
-continue;
-
-  if (header->p_type != PT_LOAD || header->p_offset != 0)
-continue;
-
-  value = value - header->p_vaddr;
-  found_offset = true;
-  break;
-}
-if (!found_offset)
+addr_t base = GetBaseAddress().GetFileAddress();
+if (base == LLDB_INVALID_ADDRESS)
   return false;
+value -= base;
   }
 
   const size_t num_sections = section_list->GetSize();
@@ -1057,6 +1046,20 @@
   return m_entry_point_address;
 }
 
+Address ObjectFileELF::GetBaseAddress() {
+  for (size_t i = 1, count = GetProgramHeaderCount(); i <= count; ++i) {
+const elf::ELFProgramHeader *header = GetProgramHeaderByIndex(i);
+if (header == nullptr)
+  continue;
+
+if (header->p_type != PT_LOAD || header->p_offset != 0)
+  continue;
+
+return header->p_vaddr;
+  }
+  return LLDB_INVALID_ADDRESS;
+}
+
 //--
 // ParseDependentModules
 //--
Index: lit/Modules/elf-section-types.yaml
===
--- lit/Modules/elf-section-types.yaml
+++ lit/Modules/elf-section-types.yaml
@@ -6,23 +6,15 @@
 
 # CHECK: Name: .debug_info
 # CHECK-NEXT: Type: dwarf-info
-# CHECK-NEXT: VM size: 0
-# CHECK-NEXT: File size: 8
 
 # CHECK: Name: .debug_types
 # CHECK-NEXT: Type: dwarf-types
-# CHECK-NEXT: VM size: 0
-# CHECK-NEXT: File size: 8
 
 # CHECK: Name: .debug_names
 # CHECK-NEXT: Type: dwarf-names
-# CHECK-NEXT: VM size: 0
-# CHECK-NEXT: File size: 8
 
 # CHECK: Name: .gnu_debugaltlink
 # CHECK-NEXT: Type: 

[Lldb-commits] [PATCH] D55422: Rename ObjectFile::GetHeaderAddress to GetBaseAddress

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348849: Rename ObjectFile::GetHeaderAddress to 
GetBaseAddress (authored by labath, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55422?vs=177165=177703#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55422

Files:
  lldb/trunk/include/lldb/Symbol/ObjectFile.h
  lldb/trunk/source/API/SBModule.cpp
  lldb/trunk/source/Commands/CommandObjectTarget.cpp
  
lldb/trunk/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
  lldb/trunk/source/Plugins/ObjectFile/JIT/ObjectFileJIT.cpp
  lldb/trunk/source/Plugins/ObjectFile/JIT/ObjectFileJIT.h
  lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
  lldb/trunk/source/Symbol/CompactUnwindInfo.cpp

Index: lldb/trunk/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
===
--- lldb/trunk/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ lldb/trunk/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -211,13 +211,13 @@
   exe_objfile->GetStrata() != ObjectFile::eStrataKernel)
 return LLDB_INVALID_ADDRESS;
 
-  if (!exe_objfile->GetHeaderAddress().IsValid())
+  if (!exe_objfile->GetBaseAddress().IsValid())
 return LLDB_INVALID_ADDRESS;
 
   if (CheckForKernelImageAtAddress(
-  exe_objfile->GetHeaderAddress().GetFileAddress(), process) ==
+  exe_objfile->GetBaseAddress().GetFileAddress(), process) ==
   exe_module->GetUUID())
-return exe_objfile->GetHeaderAddress().GetFileAddress();
+return exe_objfile->GetBaseAddress().GetFileAddress();
 
   return LLDB_INVALID_ADDRESS;
 }
@@ -931,7 +931,7 @@
   ObjectFile *kernel_object_file = m_module_sp->GetObjectFile();
   if (kernel_object_file) {
 addr_t file_address =
-kernel_object_file->GetHeaderAddress().GetFileAddress();
+kernel_object_file->GetBaseAddress().GetFileAddress();
 if (m_load_address != LLDB_INVALID_ADDRESS &&
 file_address != LLDB_INVALID_ADDRESS) {
   s->Printf("Kernel slid 0x%" PRIx64 " in memory.\n",
@@ -1005,10 +1005,10 @@
 ObjectFile *kernel_object_file = m_kernel.GetModule()->GetObjectFile();
 if (kernel_object_file) {
   addr_t load_address =
-  kernel_object_file->GetHeaderAddress().GetLoadAddress(
+  kernel_object_file->GetBaseAddress().GetLoadAddress(
   _process->GetTarget());
   addr_t file_address =
-  kernel_object_file->GetHeaderAddress().GetFileAddress();
+  kernel_object_file->GetBaseAddress().GetFileAddress();
   if (load_address != LLDB_INVALID_ADDRESS && load_address != 0) {
 m_kernel.SetLoadAddress(load_address);
 if (load_address != file_address) {
Index: lldb/trunk/source/Plugins/ObjectFile/JIT/ObjectFileJIT.cpp
===
--- lldb/trunk/source/Plugins/ObjectFile/JIT/ObjectFileJIT.cpp
+++ lldb/trunk/source/Plugins/ObjectFile/JIT/ObjectFileJIT.cpp
@@ -184,7 +184,7 @@
   return Address();
 }
 
-lldb_private::Address ObjectFileJIT::GetHeaderAddress() { return Address(); }
+lldb_private::Address ObjectFileJIT::GetBaseAddress() { return Address(); }
 
 ObjectFile::Type ObjectFileJIT::CalculateType() { return eTypeJIT; }
 
Index: lldb/trunk/source/Plugins/ObjectFile/JIT/ObjectFileJIT.h
===
--- lldb/trunk/source/Plugins/ObjectFile/JIT/ObjectFileJIT.h
+++ lldb/trunk/source/Plugins/ObjectFile/JIT/ObjectFileJIT.h
@@ -89,7 +89,7 @@
 
   lldb_private::Address GetEntryPointAddress() override;
 
-  lldb_private::Address GetHeaderAddress() override;
+  lldb_private::Address GetBaseAddress() override;
 
   ObjectFile::Type CalculateType() override;
 
Index: lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===
--- lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -1674,7 +1674,7 @@
   } else if (unified_section_sp) {
 if (is_dsym && unified_section_sp->GetFileAddress() != load_cmd.vmaddr) {
   // Check to see if the module was read from memory?
-  if (module_sp->GetObjectFile()->GetHeaderAddress().IsValid()) {
+  if (module_sp->GetObjectFile()->GetBaseAddress().IsValid()) {
 // We have a module that is in memory and needs to have its file
 // address adjusted. We need to do this because when we load a file
 // from memory, its addresses will be slid already, yet the addresses
@@ -5339,7 +5339,7 @@
   return 

[Lldb-commits] [lldb] r348849 - Rename ObjectFile::GetHeaderAddress to GetBaseAddress

[Pavel Labath via lldb-commits]

Author: labath
Date: Tue Dec 11 07:21:15 2018
New Revision: 348849

URL: http://llvm.org/viewvc/llvm-project?rev=348849=rev
Log:
Rename ObjectFile::GetHeaderAddress to GetBaseAddress

Summary:
This function was named such because in the case of MachO files, the
mach header is located at this address. However all (most?) usages of
this function were not interested in that fact, but the fact that this
address is used as the base address for expressing various relative
addresses in the object file.

For other object file formats, this name is not appropriate (and it's
probably the reason why this function was not implemented in these
classes). In the ELF case the ELF header will usually end up at this
address, but this is a result of the linker optimizing the file layout
and not a requirement of the spec. For COFF files, I believe the is no
header located at this address either.

Reviewers: clayborg, jasonmolenda, amccarth, lemo, stella.stamenova

Subscribers: lldb-commits

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

Modified:
lldb/trunk/include/lldb/Symbol/ObjectFile.h
lldb/trunk/source/API/SBModule.cpp
lldb/trunk/source/Commands/CommandObjectTarget.cpp

lldb/trunk/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
lldb/trunk/source/Plugins/ObjectFile/JIT/ObjectFileJIT.cpp
lldb/trunk/source/Plugins/ObjectFile/JIT/ObjectFileJIT.h
lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
lldb/trunk/source/Symbol/CompactUnwindInfo.cpp

Modified: lldb/trunk/include/lldb/Symbol/ObjectFile.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Symbol/ObjectFile.h?rev=348849=348848=348849=diff
==
--- lldb/trunk/include/lldb/Symbol/ObjectFile.h (original)
+++ lldb/trunk/include/lldb/Symbol/ObjectFile.h Tue Dec 11 07:21:15 2018
@@ -551,18 +551,16 @@ public:
   virtual lldb_private::Address GetEntryPointAddress() { return Address(); }
 
   //--
-  /// Returns the address that represents the header of this object file.
+  /// Returns base address of this object file.
   ///
-  /// The header address is defined as where the header for the object file is
-  /// that describes the content of the file. If the header doesn't appear in
-  /// a section that is defined in the object file, an address with no section
-  /// is returned that has the file offset set in the m_file_offset member of
-  /// the lldb_private::Address object.
-  ///
-  /// @return
-  /// Returns the entry address for this module.
+  /// This also sometimes referred to as the "preferred load address" or the
+  /// "image base address". Addresses within object files are often expressed
+  /// relative to this base. If this address corresponds to a specific section
+  /// (usually the first byte of the first section) then the returned address
+  /// will have this section set. Otherwise, the address will just have the
+  /// offset member filled in, indicating that this represents a file address.
   //--
-  virtual lldb_private::Address GetHeaderAddress() {
+  virtual lldb_private::Address GetBaseAddress() {
 return Address(m_memory_addr);
   }
 

Modified: lldb/trunk/source/API/SBModule.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/API/SBModule.cpp?rev=348849=348848=348849=diff
==
--- lldb/trunk/source/API/SBModule.cpp (original)
+++ lldb/trunk/source/API/SBModule.cpp Tue Dec 11 07:21:15 2018
@@ -587,7 +587,7 @@ lldb::SBAddress SBModule::GetObjectFileH
   if (module_sp) {
 ObjectFile *objfile_ptr = module_sp->GetObjectFile();
 if (objfile_ptr)
-  sb_addr.ref() = objfile_ptr->GetHeaderAddress();
+  sb_addr.ref() = objfile_ptr->GetBaseAddress();
   }
   return sb_addr;
 }

Modified: lldb/trunk/source/Commands/CommandObjectTarget.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Commands/CommandObjectTarget.cpp?rev=348849=348848=348849=diff
==
--- lldb/trunk/source/Commands/CommandObjectTarget.cpp (original)
+++ lldb/trunk/source/Commands/CommandObjectTarget.cpp Tue Dec 11 07:21:15 2018
@@ -2969,8 +2969,8 @@ static constexpr OptionDefinition g_targ
   { LLDB_OPT_SET_1, false, "address",'a', 
OptionParser::eRequiredArgument, nullptr, {}, 0, eArgTypeAddressOrExpression, 
"Display the image at this address." },
   { LLDB_OPT_SET_1, false, "arch",   'A', 
OptionParser::eOptionalArgument, nullptr, {}, 0, eArgTypeWidth,   
"Display the architecture when listing images." },
   { LLDB_OPT_SET_1, false, "triple", 't', 
OptionParser::eOptionalArgument, nullptr, {}, 0, 

[Lldb-commits] [PATCH] D55422: Rename ObjectFile::GetHeaderAddress to GetBaseAddress

[Pavel Labath via Phabricator via lldb-commits]

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



Comment at: source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:1677
   // Check to see if the module was read from memory?
-  if (module_sp->GetObjectFile()->GetHeaderAddress().IsValid()) {
+  if (module_sp->GetObjectFile()->GetBaseAddress().IsValid()) {
 // We have a module that is in memory and needs to have its file

clayborg wrote:
> Switching to IsInMemory will work. This code probably predates the existence 
> of IsInMemory().
Ok, I'll check this in separately, as this is the only functional change in 
this commit.


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

https://reviews.llvm.org/D55422



___
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 11/12/2018 01:08, Greg Clayton wrote:



On Dec 10, 2018, at 3:11 PM, Leonard Mosescu > wrote:


I can see how this works for the PDB, no-module-binary case. What 
about the PDB & module-binary case?


That would work fine because the symbol vendor will make an object file 
from the m_symfile_spec and use both the original binary + the symbol 
file to make symbols.


Maybe I missed the rationale, but I think that modeling the general 
case: module and symbols are separate files, is a better foundation 
for the particular case where the symbols are embedded in the binary file.


Yeah, my case should handle the following cases:
- Minidumps Placeholder modules only, no real binaries, add symbols by 
downloading them and using "target symbols add ..."
- Download real binaries up front and load them into LLDB, then load the 
core file. For any binaries we do have, we should grab them from the 
global module cache (GetTarget().GetSharedModule(...) in the current 
code) and they will use real object files, not placeholder files. 
"target symbols add ..." still works in this case




It sounds to me like it would be better to have a separate command 
(let's call it "target modules replace" for now) for adding an "object 
file" to a "placeholder" module, instead of repurposing "target symbols 
add" to do that.


This creates a strange loop between the symbol and object files -- 
normally we use the object file for symbol representation if the user 
hasn't specified a symbol file, and here, we would do the opposite.


With "target modules replace", one command would still be enough to set 
both the symbol and object files if they are the same, as the default 
use-symbols-from-object-file behavior would kick in. However, it would 
leave the "target symbols add" command free to add symbols from an 
external file.


Imagine the following scenario:
- I load a minidump, without any supporting files around
- I see that my object file is missing, I do some digging around, and 
manage to find the stripped object file for this module
- I do "target modules replace index_of_placeholder_module 
/path/to/elf.stripped"

- now my sections are there, but I still don't have symbols
- I do more digging and find the breakpad file
- I do "target symbols add /path/to/breakpad.syms"
- success. I now have both symbols and sections

Now this is probably not the most common scenario, but I think it can be 
used as a benchmark to see if the infrastructure is set up the right 
way. If things are set up correctly this should work out-of-the-box. 
With your setup, the scenario above might "just work" if I execute 
"target symbols add" twice (with different files), because the second 
"target symbols add" will do something different than the first one, but 
that sounds to me like another reason why these should be different 
commands.


Maybe there should be there should be some code to help the user and 
detect the situation when he is adding a symbol file to a placeholder 
module and the symbol file is able serve as a good object file too, but 
that code could live higher up than Module::GetObjectFile. It could 
perhaps be directly in the "target symbols add" command via something like:
if (module_I_am_about_to_add_symbols_to->IsPlaceholder() && 
symbol_file->ContainsGoodObjectFileRepresentation())

  TargetModulesReplace(module_I_am_about_to_add_symbols_to, symbol_file)
else
  the_module_I_am_about_to_add_symbols_to->SetSymbolFile(symbol_file)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

It just occurred to me that we have another copy of /proc//maps -> 
MemoryRegionInfo conversion code. It lives in NativeProcessLinux.cpp 
(ParseMemoryRegionInfoFromProcMapsLine). It would be nice to extract this 
parser into a some place (`Plugins/Process/Utility` ?), where it can be shared 
by the two process plugins. As a bonus, that should make it easy to write unit 
tests for that function.


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

https://reviews.llvm.org/D55522



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