[Lldb-commits] [PATCH] D48623: Move architecture-specific address adjustment to architecture plugins.

[Greg Clayton via Phabricator via lldb-commits]

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

much better! Thanks for making the changes


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D48623



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

[Lldb-commits] [PATCH] D52403: [LLDB] - Support the single file split DWARF.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

So the main questions is: do we need a new section enum called 
eSectionTypeDWARFDebugInfoDWO? If so, then this patch changes. I think I would 
prefer adding a new enum.




Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp:68
-if (auto section_sp =
-section_list->FindSectionByType(eSectionTypeDWARFDebugInfo, true))
-  if (!section_sp->GetName().GetStringRef().endswith("dwo"))

If dwo has a unique section name, just name a new enum for the .debug_info.dwo.


https://reviews.llvm.org/D52403



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

[Lldb-commits] [PATCH] D52406: Make DIE_IS_BEING_PARSED local to the current thread.

[Greg Clayton via Phabricator via lldb-commits]

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

A little background might help here: The lldb_private::Module lock is used to 
prevent multiple queries into the DWARF from stomping on each other.

Multi-threaded DWARF parsing was primarily added to speed up indexing and the 
only place it is used. Is that not true? Indexing is just creating name tables 
and the accelerator tables we need so that we can partially parse the DWARF 
later. No type parsing should be happening when indexing. All other accesses to 
the DWARF must be done via a SymbolFile API that takes the module lock to stop 
multiple threads from stomping on each other.

This fix will not work for a few reasons:

- multiple threads should not be allowed to parse types at the same time, the 
module lock must be taken, if that isn't the case, then this needs to be fixed
- even if we were to allow this fix and if multiple threads are able to parse 
DWARF at the same time, this fix wouldn't work. We have one clang AST per 
symbol and we convert DWARF into clang ASTs. We can't have multiple threads 
creating a "class Foo" at the same time and populating them at anytime.

So my main point is we need to use the module lock to avoid having multiple 
threads doing important work that will cause crashes.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52406



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

[Lldb-commits] [PATCH] D48393: Make DWARFParsing more thread-safe

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

A little background might help here: The lldb_private::Module lock is used to 
prevent multiple queries into the DWARF from stomping on each other.

Multi-threaded DWARF parsing was primarily added to speed up indexing and the 
only place it is used. Is that not true? Indexing is just creating name tables 
and the accelerator tables we need so that we can partially parse the DWARF 
later. No type parsing should be happening when indexing. All other accesses to 
the DWARF must be done via a SymbolFile API that takes the module lock to stop 
multiple threads from stomping on each other. So my main point is we need to 
use the module lock to avoid having multiple threads doing important work that 
will cause crashes. The only multi-threaded access to DWARF currently should be 
in the indexing only and no important parsing stuff should be going on. If we 
discover places where multiple threads are making accesses, we just need to 
ensure they take the module lock and everything should just work.

In https://reviews.llvm.org/D48393#1139290, @zturner wrote:

> Related question: Is the laziness done to save memory, startup time, or both?


Both. Memory is the bigger issue here though. We want to only parse what we 
need when we need it so if someone was to use LLDB for symbolication, they 
don't incur the cost of parsing all types in any compile units that get touched 
(like would happen with GDB). Since we convert all DWARF types into clang AST 
types, parsing more types than needed can cause a lot of memory to be used.


https://reviews.llvm.org/D48393



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

[Lldb-commits] [PATCH] D52375: [WIP] Support multiple compile units per OSO entry in SymbolFileDWARFDebugMap

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp:298-306
+static uint32_t GetOSOSymbolFlags() {
+  // When a mach-o symbol is encoded, the n_type field is encoded in bits
+  // 23:16, and the n_desc field is encoded in bits 15:0.
+  //
+  // N_OSO object file symbols have a flags value as follows:
+  // bits 23:16 == 0x66 (N_OSO)
+  // bits 15: 0 == 0x0001 (specifies this is a debug map object file)

```
static constexpr uint32_t kOSOSymbolFlags = 0x660001u;
```



Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp:328
 std::vector oso_indexes;
-// When a mach-o symbol is encoded, the n_type field is encoded in bits
-// 23:16, and the n_desc field is encoded in bits 15:0.
-//
-// To find all N_OSO entries that are part of the DWARF + debug map we find
-// only object file symbols with the flags value as follows: bits 23:16 ==
-// 0x66 (N_OSO) bits 15: 0 == 0x0001 (specifies this is a debug map object
-// file)
-const uint32_t k_oso_symbol_flags_value = 0x660001u;
+const uint32_t k_oso_symbol_flags_value = GetOSOSymbolFlags();
 

kOSOSymbolFlags



Comment at: source/Symbol/Symtab.cpp:512-520
+bool Symtab::HasSymbolWithTypeAndFlags(lldb::SymbolType symbol_type,
+   uint32_t flags_value) const {
+  std::lock_guard guard(m_mutex);
+  for (const Symbol &symbol : m_symbols)
+if (symbol.GetType() == symbol_type && symbol.GetFlags() == flags_value)
+  return true;
+

This function is a bit specific. Might be nice to have a method like:
```
void Symtab::ForEachSymbolWithType(
  lldb::SymbolType symbol_type, 
  llvm::function_ref lambda) const;
```

The lambda returns false, then stop iterating. Else keep going. See 
ModuleList::ForEach as an example. Then the code that was calling 
HasSymbolWithTypeAndFlags would do something like:

```
symtab->ForEachSymbolWithType(eSymbolTypeObjectFile, 
[&this->m_has_compile_unit_infos](Symbol *symbol)->bool {
  if (symbol->GetFlags() == kOSOSymbolFlags) {
m_has_compile_unit_infos = true;
return false; // Stop iterating
  }
  return true; // Keep iterating
});
```


https://reviews.llvm.org/D52375



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

[Lldb-commits] [PATCH] D52375: [WIP] Support multiple compile units per OSO entry in SymbolFileDWARFDebugMap

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:431
 const size_t num_ranges =
-die->GetAttributeAddressRanges(dwarf, this, ranges, false);
+die->GetAttributeAddressRanges(dwarf, this, ranges, check_hi_lo_pc);
 if (num_ranges > 0) {

sgraenitz wrote:
> @JDevlieghere Thanks for your feedback!
> 
> @everyone:
> This parameter is, actually, one of the key questions in this review: If 
> there is no DW_AT_range attribute for the compile unit, can we safely fall 
> back to DW_AT_low/high_pc?
I have seen DWARF where the DW_AT_ranges is incorrect and I have seen cases 
where the low pc is just set to zero and the high pc is set to the max address. 
So you might have many overlapping ranges between different CUs in the latter 
case. We might need to be more vigilant with high/low pc values though and that 
was the reason that we previously ignored high/low pc values. If the stuff is 
missing, it should index the DWARF and generate the address ranges table 
manually right? What was the reasoning behind this change?


https://reviews.llvm.org/D52375



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

[Lldb-commits] [PATCH] D48393: Make DWARFParsing more thread-safe

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In https://reviews.llvm.org/D48393#1244426, @JDevlieghere wrote:

> Thanks for the information, Greg!
>
> In https://reviews.llvm.org/D48393#1243588, @clayborg wrote:
>
> > A little background might help here: The lldb_private::Module lock is used 
> > to prevent multiple queries into the DWARF from stomping on each other.
> >
> > Multi-threaded DWARF parsing was primarily added to speed up indexing and 
> > the only place it is used. Is that not true? Indexing is just creating name 
> > tables and the accelerator tables we need so that we can partially parse 
> > the DWARF later. No type parsing should be happening when indexing. All 
> > other accesses to the DWARF must be done via a SymbolFile API that takes 
> > the module lock to stop multiple threads from stomping on each other. So my 
> > main point is we need to use the module lock to avoid having multiple 
> > threads doing important work that will cause crashes.
>
>
> Looking at `SymbolFileDWARF`, we only have two methods that take the module 
> lock: `PreloadSymbols` and `CompleteType`.


Most of the locking happens at the SymbolVendor level IIRC. But all virtual 
function SymbolFile entry points need to take the module lock.

> 
> 
>> The only multi-threaded access to DWARF currently should be in the indexing 
>> only and no important parsing stuff should be going on.
> 
> Surely there can be a lot more going on, like two debuggers executing an 
> expression at the same time?

That doesn't matter. One of them will index the DWARF. Both will be able to 
field requests via the virtual functions defined in lldb_private::SymbolFile 
(usually via the lldb_private::SymbolVendor (a class that can combine multiple 
object files/symbol files)) so any query must use the module lock. This is no 
different than two threads making two different requests from a SymbolFileDWARF.

> 
> 
>> If we discover places where multiple threads are making accesses, we just 
>> need to ensure they take the module lock and everything should just work.
> 
> So what you're saying is that we should add the module lock to every endpoint 
> in `SymbolFileDWARF` that can potentially modify our internal data structures 
> (i.e. parsing)?

Yes, any virtual lldb_private::SymbolFile entry point should lock the recursive 
module mutex. Check SymbolVendor as no one directly grabs a SymbolFile and uses 
it, they all go through the module which uses the symbol vendor. So the locking 
will often occur at the module level or the symbol vendor level.


https://reviews.llvm.org/D48393



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

[Lldb-commits] [PATCH] D48393: Make DWARFParsing more thread-safe

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In https://reviews.llvm.org/D48393#1244649, @labath wrote:

> I agree with Greg that it would be best to restrict things such that there is 
> only one instance of parsing going on at any given time for a single module. 
> I think this was pretty much the state we reached when this thread fizzled 
> out the last time (there are some extra emails that are not showing in 
> phabricator history, the interesting ones start around here: 
> http://lists.llvm.org/pipermail/lldb-commits/Week-of-Mon-20180618/041937.html).
>  I think the main part that needed to be resolved is whether we need to go 
> anything special when resolving debug info references *between* modules (e.g. 
> to prevent A/B deadlocks).


We don't have to worry about references between modules as no module loads 
_ANY_ debug info from another module. If a module only has a forward 
declaration to a type, we leave it as a forward declaration. Why do we do this? 
So we can cache modules in LLDB and use all the parsed state in the next debug 
session if we rerun. Lets say you have liba.so that has a type "struct Foo { 
int x; };" and libb.so has a "struct Foo; Foo *foo_ptr;". libb.so has debug 
info for "foo_ptr" that says it is a forward declaration. At expression 
evaluation time, we will end up digging up the correct definition for "struct 
Foo" from liba.so, but we must do that each time we evaluate an expression. If 
we change liba.so with "struct Foo { int x; int y; };" and re-run, we don't 
need to reload any debug info for libb.so (or keep track of any definitions we 
might have loaded from another file to know when liba.so changes that we need 
to invalidate any number of types in other shared libraries). So because of 
this, we don't run into the problem. The variable displaying infrastructure in 
ValueObject and the expression parser know how to locate types when they need 
to. This keeps everything clean in the debug info parsing code.


https://reviews.llvm.org/D48393



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

[Lldb-commits] [PATCH] D52461: [PDB] Introduce `PDBNameParser`

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In https://reviews.llvm.org/D52461#1244813, @labath wrote:

> I think you should look at CPlusPlusLanguage::MethodName. It already contains 
> a parser (in fact, two of them) of c++ names, and I think it should be easy 
> to extend it to do what you want.


I agree with Pavel here. Try to use and extend CPlusPlusLanguage::MethodName as 
needed. I believe it was recently backed by a new clang parser that knows how 
to chop up C++ demangled names


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52461



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

[Lldb-commits] [PATCH] D52501: [PDB] Restore the calling convention from PDB

[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 as long as all the test suite bots are happy.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52501



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

[Lldb-commits] [PATCH] D48393: Make DWARFParsing more thread-safe

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In https://reviews.llvm.org/D48393#1245342, @friss wrote:

> In https://reviews.llvm.org/D48393#1245049, @clayborg wrote:
>
> > In https://reviews.llvm.org/D48393#1244649, @labath wrote:
> >
> > > I agree with Greg that it would be best to restrict things such that 
> > > there is only one instance of parsing going on at any given time for a 
> > > single module. I think this was pretty much the state we reached when 
> > > this thread fizzled out the last time (there are some extra emails that 
> > > are not showing in phabricator history, the interesting ones start around 
> > > here: 
> > > http://lists.llvm.org/pipermail/lldb-commits/Week-of-Mon-20180618/041937.html).
> > >  I think the main part that needed to be resolved is whether we need to 
> > > go anything special when resolving debug info references *between* 
> > > modules (e.g. to prevent A/B deadlocks).
> >
> >
> > We don't have to worry about references between modules as no module loads 
> > _ANY_ debug info from another module.
>
>
> What about -gmodules? I don't remember where the module debug info is stored, 
> I think it is not shared so your statement would conceptually hold, but I'm 
> not sure anymore.


We load the -gmodules DWARF and a stand alone file and then copy the AST types 
over into the AST for the SymbolFileDWARF that uses them. So we are good with 
-gmodules.


https://reviews.llvm.org/D48393



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

[Lldb-commits] [PATCH] D52375: [WIP] Support multiple compile units per OSO entry in SymbolFileDWARFDebugMap

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:425
   const DWARFDebugInfoEntry *die = GetUnitDIEPtrOnly();
 
   const dw_offset_t cu_offset = GetOffset();

Following from inline comment below, you can abort if this CU is empty:

```
// If this DWARF unit has no children, we have no address ranges to add. If
// something is built with line tables only, there will still be function DIEs 
with
// address ranges as children.
if (!die->HasChildren())
  return;
```



Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:439
+die->GetAttributeAddressRanges(dwarf, this, ranges, check_hi_lo_pc);
+
 if (num_ranges > 0) {

Empty CUs won't have any children. Easy to skip by getting the CU die and doing:
```
if (!die->HasChildren())
```




Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:488-489
 }
-  } else
-debug_map_sym_file->AddOSOARanges(dwarf, debug_aranges);
+  } //else
+//debug_map_sym_file->AddOSOARanges(dwarf, debug_aranges);
 }

revert if the "if (!die->hasChildren())" trick works.


https://reviews.llvm.org/D52375



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

[Lldb-commits] [PATCH] D52543: [DWARFSymbolFile] Adds the module lock to all virtual methods from SymbolFile

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

It is the SymbolVendor's job to do the locking. And in many cases it already 
is. I stopped with inlined comments after a few comments as it would apply to 
this entire patch.

It would be great if we can replace all of the locations where you added lock 
guards with lldbasserts to assert that the lock is already taken. Then we might 
be able to catch places where the locks are not being respected. But in general 
the symbol vendor should take the lock, use N number of SymbolFile instances to 
complete the request, and then release the lock. Makes things easier on the 
lldb_private::SymbolFile subclasses this way.




Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:266-267
 TypeList *SymbolFileDWARF::GetTypeList() {
+  std::lock_guard guard(
+  GetObjectFile()->GetModule()->GetMutex());
   SymbolFileDWARFDebugMap *debug_map_symfile = GetDebugMapSymfile();

This shouldn't require locking as it is just handing out a type list ivar from 
one place or another.



Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:277-278
TypeSet &type_set) {
+  std::lock_guard guard(
+  GetObjectFile()->GetModule()->GetMutex());
   if (die) {

This is not a SymbolFile override. It shouldn't require the locking.



Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:442-443
 TypeSystem *SymbolFileDWARF::GetTypeSystemForLanguage(LanguageType language) {
+  std::lock_guard guard(
+  GetObjectFile()->GetModule()->GetMutex());
   SymbolFileDWARFDebugMap *debug_map_symfile = GetDebugMapSymfile();

This shouldn't require locking as it is just handing out a type system from one 
place or another.



Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:857-858
 uint32_t SymbolFileDWARF::GetNumCompileUnits() {
+  std::lock_guard guard(
+  GetObjectFile()->GetModule()->GetMutex());
   DWARFDebugInfo *info = DebugInfo();

SymbolVendor::GetNumCompileUnits() already takes the module lock for us. All 
other internal calls to this should be protected as well



Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:866-867
 CompUnitSP SymbolFileDWARF::ParseCompileUnitAtIndex(uint32_t cu_idx) {
+  std::lock_guard guard(
+  GetObjectFile()->GetModule()->GetMutex());
   CompUnitSP cu_sp;

SymbolVendor already takes the module lock for us



Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:880-881
 const DWARFDIE &die) {
+  std::lock_guard guard(
+  GetObjectFile()->GetModule()->GetMutex());
   if (die.IsValid()) {

SymbolVendor requests that lead to this call should take the lock for us...


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52543



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

[Lldb-commits] [PATCH] D52572: Replace pointer to C-array of PropertyDefinition with llvm::ArrayRef

[Greg Clayton via Phabricator via lldb-commits]

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

Good stuff!


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52572



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

[Lldb-commits] [PATCH] D52604: Clean-up usage of OptionDefinition arrays

[Greg Clayton via Phabricator via lldb-commits]

clayborg accepted this revision.
clayborg added inline comments.
This revision is now accepted and ready to land.



Comment at: tools/driver/Driver.cpp:71
 
 typedef struct {
   uint32_t usage_mask; // Used to mark options that can be used together.  If 
(1

tatyana-krasnukha wrote:
> This type is duplicated here to not break the encapsulation of lldb's private 
> types, am I right?
Correct. We want any tool that links against LLDB.framework on darwin, or 
liblldb.so on other platforms to use the lldb::SB API only. 



Comment at: tools/driver/Driver.cpp:581-585
+  if (g_num_options == 0) {
 if (argc > 1)
   error.SetErrorStringWithFormat("invalid number of options");
 return error;
   }

Do we even need this if statement and its contents? maybe 
lldbassert(g_num_options > 0);?


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52604



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

[Lldb-commits] [PATCH] D52403: [LLDB] - Support the single file split DWARF.

[Greg Clayton via Phabricator via lldb-commits]

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

Just a few fixes. Looking good.




Comment at: include/lldb/lldb-enumerations.h:643-660
+  eSectionTypeDWARFDebugAbbrevDwo,
   eSectionTypeDWARFDebugAddr,
   eSectionTypeDWARFDebugAranges,
   eSectionTypeDWARFDebugCuIndex,
   eSectionTypeDWARFDebugFrame,
   eSectionTypeDWARFDebugInfo,
+  eSectionTypeDWARFDebugInfoDwo,

Add all of these to the end of this enum for API stability since this is a 
public header used in the API. If an older binary runs against a newer 
liblldb.so, all of these enums will be off.



Comment at: source/Symbol/ObjectFile.cpp:347
   case eSectionTypeDWARFDebugAbbrev:
+  case eSectionTypeDWARFDebugAbbrevDwo:
   case eSectionTypeDWARFDebugAddr:

Check for other ObjectFile subclasses that override this function. I believe 
ObjectFileMachO does.


https://reviews.llvm.org/D52403



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

[Lldb-commits] [PATCH] D52375: [WIP] Support multiple compile units per OSO entry in SymbolFileDWARFDebugMap

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: source/Symbol/Symtab.cpp:520
+  return false;
+}
+

This function is still pretty specific. Any comments on my above inlined 
comment?


https://reviews.llvm.org/D52375



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

[Lldb-commits] [PATCH] D52618: [Windows] A basic implementation of memory allocations in a debuggee process

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

We really should be making a lldb-server that works on windows instead of 
making a native windows process plug-in that only works on windows. That will 
allow remote debugging to windows machines instead of requiring a local 
connection. It will also allows debug sessions to be logged using the GDB 
remote packets and avoids all of the debugging that goes on with a plug-in 
(ProcessWindows) that does event notifications and everything differently than 
other targets.


https://reviews.llvm.org/D52618



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

[Lldb-commits] [PATCH] D52375: [WIP] Support multiple compile units per OSO entry in SymbolFileDWARFDebugMap

[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 then as long as all tests pass on Darwin.


https://reviews.llvm.org/D52375



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

[Lldb-commits] [PATCH] D52403: [LLDB] - Support the single file split DWARF.

[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!


https://reviews.llvm.org/D52403



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

[Lldb-commits] [PATCH] D52678: DWARFExpression: Resolve file addresses in the linked module

[Greg Clayton via Phabricator via lldb-commits]

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

Any idea why the module isn't set correctly in certain dwarf expressions? Any 
variable that is created from debug info that pertains to a module should have 
its DWARFExpression created with the correct module. A DWARFExpression that has 
no module, yet it has file addresses with DW_OP_addr, was not created 
correctly. If you grab a global variable from module "a.out" and create a 
DWARFExpression that doesn't have its module set correctly, but we are stopped 
in libfoo.dylib in some frame inside there, and then evaluate the expression it 
will evaluate incorrectly or not at all if the file address doesn't match 
something in the current frame. The right fix is to ensure the DWARFExpression 
is created correctly with a module.


https://reviews.llvm.org/D52678



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

[Lldb-commits] [PATCH] D52678: DWARFExpression: Resolve file addresses in the linked module

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

See SymbolFileDWARF::ParseVariableDIE. It has code that links up the DW_OP_addr:

  if (is_static_lifetime) {
if (is_external)
  scope = eValueTypeVariableGlobal;
else
  scope = eValueTypeVariableStatic;
  
if (debug_map_symfile) {
  // When leaving the DWARF in the .o files on darwin, when we have a
  // global variable that wasn't initialized, the .o file might not
  // have allocated a virtual address for the global variable. In
  // this case it will have created a symbol for the global variable
  // that is undefined/data and external and the value will be the
  // byte size of the variable. When we do the address map in
  // SymbolFileDWARFDebugMap we rely on having an address, we need to
  // do some magic here so we can get the correct address for our
  // global variable. The address for all of these entries will be
  // zero, and there will be an undefined symbol in this object file,
  // and the executable will have a matching symbol with a good
  // address. So here we dig up the correct address and replace it in
  // the location for the variable, and set the variable's symbol
  // context scope to be that of the main executable so the file
  // address will resolve correctly.

This is what needs to be fixed.


https://reviews.llvm.org/D52678



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

[Lldb-commits] [PATCH] D52678: DWARFExpression: Resolve file addresses in the linked module

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

If this isn't fixing up cases where the global does have an address (not just 
set to zero), we need to fix the code so it relinks the global address 
correctly.


https://reviews.llvm.org/D52678



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

[Lldb-commits] [PATCH] D52678: DWARFExpression: Resolve file addresses in the linked module

[Greg Clayton via Phabricator via lldb-commits]

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

Jim: we are already linking the address for the DW_OP_addr using the debug map 
and no .o files are currently expected to be able to link an unlinked address 
into a file address as nothing in the .o file knows about the existence of the 
debug map object file, so this is the right fix.


https://reviews.llvm.org/D52678



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

[Lldb-commits] [PATCH] D52981: [LLDB] - Add basic support for .debug_rnglists section (DWARF5)

[Greg Clayton via Phabricator via lldb-commits]

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

Just add a switch statement when handling the encodings and a lldbassert as 
mentioned in inlined comments and this will be good to go.




Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.cpp:134-144
+uint8_t encoding = data.GetU8(offset_ptr);
+
+if (encoding == DW_RLE_end_of_list)
+  return true;
+
+if (encoding == DW_RLE_start_length) {
+  dw_addr_t begin = data.GetMaxU64(offset_ptr, addrSize);

Use a switch statement here? We also want to use a lldbassert for any non 
supported encodings (in the default case of the switch statement) so we know 
if/when a compiler starts emitting an encoding we don't yet support when 
running the test suite with assertions enabled. That will let us know why 
things are failing. 


https://reviews.llvm.org/D52981



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

[Lldb-commits] [PATCH] D52981: [LLDB] - Add basic support for .debug_rnglists section (DWARF5)

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

For space savings it seems like we would want to support the 
DW_RLE_base_address and DW_RLE_offset_pair pretty soon.


https://reviews.llvm.org/D52981



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

[Lldb-commits] [PATCH] D52689: [LLDB] - Add support for DW_FORM_implicit_const.

[Greg Clayton via Phabricator via lldb-commits]

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

See inline comments.




Comment at: source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h:51
+  uint32_t idx, dw_attr_t &attr, dw_form_t &form,
+  DWARFFormValue::ValueType *val = nullptr) const {
+m_attributes[idx].get(attr, form, val);

Switch to using a "DWARFFormValue *form_value_ptr" so the form value can be 
filled in automatically, not just the   DWARFFormValue::ValueType. See 
comments below where this is called.



Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:386
 
+static void setUnsignedOrSigned(int &dest, DWARFFormValue &val) {
+  if (val.Form() == DW_FORM_implicit_const)

Remove this as it won't be needed if we do the work of filling in the form 
value in GetAttrAndFormByIndexUnchecked



Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:435
 dw_form_t form;
+DWARFFormValue::ValueType val;
 bool do_offset = false;

Remove? See inlined comment below.



Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:439-440
 for (i = 0; i < numAttributes; ++i) {
-  abbrevDecl->GetAttrAndFormByIndexUnchecked(i, attr, form);
-  DWARFFormValue form_value(cu, form);
+  abbrevDecl->GetAttrAndFormByIndexUnchecked(i, attr, form, &val);
+  DWARFFormValue form_value(cu, form, val);
+

Maybe switch the order here and pass "form_value" to 
GetAttrAndFormByIndexUnchecked?:

```
  DWARFFormValue form_value(cu, form);
  abbrevDecl->GetAttrAndFormByIndexUnchecked(i, attr, form, &form_value);
```




Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:508
   if (decl_file == 0)
-decl_file = form_value.Unsigned();
+setUnsignedOrSigned(decl_file, form_value);
   break;

Revert this if we do the work inside "GetAttrAndFormByIndexUnchecked" as 
form_value will be correct



Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:512
 case DW_AT_decl_line:
-  if (decl_line == 0)
-decl_line = form_value.Unsigned();
+  if (decl_line != 0)
+setUnsignedOrSigned(decl_line, form_value);

You changed the logic here incorrectly from == to !=



Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:513
+  if (decl_line != 0)
+setUnsignedOrSigned(decl_line, form_value);
   break;

Revert this if we do the work inside "GetAttrAndFormByIndexUnchecked" as 
form_value will be correct



Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:517
 case DW_AT_decl_column:
-  if (decl_column == 0)
-decl_column = form_value.Unsigned();
+  if (decl_column != 0)
+setUnsignedOrSigned(decl_column, form_value);

You changed the logic here incorrectly from == to !=



Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:518
+  if (decl_column != 0)
+setUnsignedOrSigned(decl_column, form_value);
   break;

Revert this if we do the work inside "GetAttrAndFormByIndexUnchecked" as 
form_value will be correct



Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:522
 case DW_AT_call_file:
-  if (call_file == 0)
-call_file = form_value.Unsigned();
+  if (call_file != 0)
+setUnsignedOrSigned(call_file, form_value);

You changed the logic here incorrectly from == to !=



Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:523
+  if (call_file != 0)
+setUnsignedOrSigned(call_file, form_value);
   break;

Revert this if we do the work inside "GetAttrAndFormByIndexUnchecked" as 
form_value will be correct



Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:528
   if (call_line == 0)
-call_line = form_value.Unsigned();
+setUnsignedOrSigned(call_line, form_value);
   break;

Revert this if we do the work inside "GetAttrAndFormByIndexUnchecked" as 
form_value will be correct



Comment at: source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp:157-159
+DWARFFormValue::DWARFFormValue(const DWARFUnit *cu, dw_form_t form,
+   ValueType val)
+: m_cu(cu), m_form(form), m_value(val) {}

Revert this if we do the work inside "GetAttrAndFormByIndexUnchecked" as 
form_value will be filled in by that call





Comment at: source/Plugins/SymbolFile/DWARF/DWARFFormVa

[Lldb-commits] [PATCH] D52689: [LLDB] - Add support for DW_FORM_implicit_const.

[Greg Clayton via Phabricator via lldb-commits]

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

A few things inlined. Very close. DumpAttribute will need to take a 
DWARFFormValue in order to dump the value correctly.




Comment at: source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h:43-44
+dw_form_t form;
+DWARFFormValue::ValueType val;
+m_attributes[idx].get(attr, form, val);
+form_value.SetForm(form);

It would be nice to be able to access the dw_form_t and 
DWARFFormValue::ValueType within "form_value" without having to create a temp 
variables here for both "form" and "val". Fine to add accessors that return 
references for the form_t and DWARFFormValue::ValueType to DWARFFormValue.



Comment at: source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h:45-46
+m_attributes[idx].get(attr, form, val);
+form_value.SetForm(form);
+form_value.SetValue(val);
   }

If we are able to follow my last inline comment, then these go away.



Comment at: source/Plugins/SymbolFile/DWARF/DWARFAttribute.h:23
+  DWARFAttribute(dw_attr_t attr, dw_form_t form,
+ DWARFFormValue::ValueType value)
+  : m_attr(attr), m_form(form), m_value(value) {}

Do we need to use a "DWARFFormValue::ValueType" here? Right now we only need a 
int64_t and DWARFFormValue::ValueType is larger than that. It does future proof 
us a bit and there aren't all that many abbreviations, even in a large DWARF 
file. Just thinking out loud here



Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:630-631
 
   DumpAttribute(dwarf2Data, cu, debug_info_data, &offset, s, attr,
-form);
+form_value.Form());
 }

DumpAttribute will need to take the full form_value in order to dump 
DW_FORM_implicit_const forms correctly. Change DumpAttribute to take a 
"form_value"


https://reviews.llvm.org/D52689



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

[Lldb-commits] [PATCH] D52689: [LLDB] - Add support for DW_FORM_implicit_const.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Down to just modifying the DWARFFormValue constructor to be able to take a CU 
only. Looks good.




Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:241-242
+for (uint32_t i = 0; i < numAttributes; ++i) {
+  DWARFFormValue form_value;
+  form_value.SetCompileUnit(cu);
+

Make either a constructor that takes just a CU or provide a default value for 
the dw_form_t form? Then this code will be:

```
DWARFFormValue form_value(cu);
```



Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:429-430
+for (uint32_t i = 0; i < numAttributes; ++i) {
+  DWARFFormValue form_value;
+  form_value.SetCompileUnit(cu);
+

Make either a constructor that takes just a CU or provide a default value for 
the dw_form_t form? Then this code will be:

```
DWARFFormValue form_value(cu);
```



Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:625-626
+for (uint32_t i = 0; i < numAttributes; ++i) {
+  DWARFFormValue form_value;
+  form_value.SetCompileUnit(cu);
+  dw_attr_t attr;

Make either a constructor that takes just a CU or provide a default value for 
the dw_form_t form? Then this code will be:

```
DWARFFormValue form_value(cu);
```



Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:803-804
+for (uint32_t i = 0; i < num_attributes; ++i) {
+  DWARFFormValue form_value;
+  form_value.SetCompileUnit(cu);
+

Make either a constructor that takes just a CU or provide a default value for 
the dw_form_t form? Then this code will be:

```
DWARFFormValue form_value(cu);
```


https://reviews.llvm.org/D52689



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

[Lldb-commits] [PATCH] D52689: [LLDB] - Add support for DW_FORM_implicit_const.

[Greg Clayton via Phabricator via lldb-commits]

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

Thanks for making the changes. Looks good!


https://reviews.llvm.org/D52689



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

[Lldb-commits] [PATCH] D53140: [LLDB] - Add support for DW_RLE_base_address and DW_RLE_offset_pair entries (.debug_rnglists)

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

See inlined comments.




Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.cpp:150-151
+case DW_RLE_base_address: {
+  dw_addr_t base = data.GetMaxU64(offset_ptr, addrSize);
+  rangeList.push_back({DW_RLE_base_address, base, 0});
+  break;

Might be nice to not push a base address entry and store the base address 
locally and fixup any DW_RLE_offset_pair entries on the fly?



Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.h:51-55
+  struct RngListEntry {
+uint8_t encoding;
+uint64_t value0;
+uint64_t value1;
+  };

Do we really need to store all this? Can't we just convert to address ranges on 
the fly in DWARFDebugRngLists::Extract? With the current DW_RLE_base_address 
and DW_RLE_offset_pair stuff we can store the base address locally inside the 
DWARFDebugRngLists::Extract function and skip pushing an entry for it and then 
convert any DW_RLE_offset_pair stuff into addresses by adding the base address 
before pushing the range. Or will this be required to support other opcodes?


https://reviews.llvm.org/D53140



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

[Lldb-commits] [PATCH] D53297: [lldbsuite] Make the names of test classes unique

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

It would be great if we can detect this when all of the test files are loaded 
and emit an error instead of waiting for results to be overwritten


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53297



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

[Lldb-commits] [PATCH] D53193: [LLDB] - Add support for DW_RLE_start_end entries (.debug_rnglists)

[Greg Clayton via Phabricator via lldb-commits]

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

LGTM


https://reviews.llvm.org/D53193



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

[Lldb-commits] [PATCH] D53321: Code cleanup: Remove DWARFDebugInfoEntry::m_empty_children

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

m_has_children was used to represent what was in the DWARF in the byte that 
follows the DW_TAG. m_empty_children was used for DIEs that said they had 
children but actually only contain a single NULL tag. It is fine to not 
differentiate between the two.




Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h:282
  // If zero this die has no parent
-  uint32_t m_sibling_idx : 31, // How many to add to "this" to get the sibling.
-  m_empty_children : 1;// If a DIE says it had children, yet it just
-   // contained a NULL tag, this will be set.
+  uint32_t m_sibling_idx; // How many to add to "this" to get the sibling.
   uint32_t m_abbr_idx : DIE_ABBR_IDX_BITSIZE,

We might want to move m_has_children where m_empty_children used to be and give 
a bit back to m_abbr_idx by increasing DIE_ABBR_IDX_BITSIZE by 1. We have a few 
bits to spare in m_sibling_idx since it is the number of dies to advance by to 
get to the sibling. Since DIEs at the very least are a few bytes, having a 2GB 
advance instead of 4GB is plenty.



Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h:285-287
+// It is zero if a DIE says it had
+// children, yet it just contained
+// a NULL tag.

This comment is wrong. The DWARF encodes if a DIE has children in the 
.debug_info, but we may override this value if the DIE only contains an NULL 
terminating DIE.

It should read something like:
```
// If it is zero, then the DIE doesn't have children, or the 
// DWARF claimed it had children but the DIE only contained 
// a single NULL terminating child.
```




Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53321



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

[Lldb-commits] [PATCH] D53321: Code cleanup: Remove DWARFDebugInfoEntry::m_empty_children

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

See inlined comment.




Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h:287
+  m_has_children : 1;
   uint32_t m_abbr_idx : DIE_ABBR_IDX_BITSIZE,
 m_tag : 16; // A copy of the DW_TAG value so we don't

Might be worth changing these to be uint16_t, removing DIE_ABBR_IDX_BITSIZE and 
changing any code that was using that to make sure that the value is <= 
UINT16_MAX:

```
uint16_t m_abbr_idx;
uint16_t m_tag;
```


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53321



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

[Lldb-commits] [PATCH] D53321: Code cleanup: Remove DWARFDebugInfoEntry::m_empty_children

[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. Nice cleanup.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53321



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

[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Since this is duplicated in so many spots, can we make a helper function to do 
this to ensure no one gets it wrong. We might be able to pass in the "guard" 
and "stop_locker" as reference variables and modify them in the helper function.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53412



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

[Lldb-commits] [PATCH] D52543: [DWARFSymbolFile] Add the module lock where necessary and assert that we own it.

[Greg Clayton via Phabricator via lldb-commits]

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

See inlined comments




Comment at: include/lldb/Symbol/SymbolFile.h:98
 
+  virtual std::recursive_mutex &GetModuleMutex() const;
+

Might add a comment here stating something like "Symbols file subclasses should 
override this to return the Module that owns the TypeSystem that this symbol 
file modifies type information in".



Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2022-2025
+  if (m_debug_map_module_wp.expired())
+return GetObjectFile()->GetModule()->GetMutex();
+  lldb::ModuleSP module_sp(m_debug_map_module_wp.lock());
+  return module_sp->GetMutex();

Possible race condition and crasher here. Best coded as:

```
  lldb::ModuleSP module_sp(m_debug_map_module_wp.lock());
  if (module_sp)
return module_sp->GetMutex();
  return GetObjectFile()->GetModule()->GetMutex();
```

Otherwise some other thread could clear "m_debug_map_module_wp" after 
"m_debug_map_module_wp.expired()" is called, but before 
"m_debug_map_module_wp.lock()" is called and we would crash.



Comment at: source/Symbol/SymbolFile.cpp:160-168
+void SymbolFile::AssertModuleLock() {
+  // We assert that we have to module lock by trying to acquire the lock from a
+  // different thread. Note that we must abort if the result is true to
+  // guarantee correctness.
+  lldbassert(std::async(std::launch::async,
+[this] { return this->GetModuleMutex().try_lock(); })
+ .get() == false &&

We should make this entire thing and all uses of it into a macro so we can 
enable it for Debug builds, but disable it for Release builds and have no extra 
code in release builds. We really shouldn't be starting threads up all the time 
for every function in SymbolFile subclasses for Release builds. It is ok for 
Debug builds or maybe manually enable this when we run into issues, but this is 
too expensive to leave in for all builds.


https://reviews.llvm.org/D52543



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

[Lldb-commits] [PATCH] D53368: [Symbol] Search symbols with name and type in a symbol file

[Greg Clayton via Phabricator via lldb-commits]

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

All symbol tables are currently extracted from the object files via 
ObjectFile::GetSymtab(). Are symbols only in the PDB file? If so I would vote 
to add a "virtual void SymbolVendor::AddSymbols(Symtab *symtab)" and a "virtual 
void SymbolFile::AddSymbols(Symtab *symtab)" where we take the symbol table 
that comes from the object file and we can add symbols to it if the symbol file 
has symbols it wants to add to the object file's symbol table. All symbol 
queries go through the lldb_private::Symtab class anyway. Care must be taken to 
watch out for symbols that might already exist from an ObjectFile's symbol 
table to ensure don't have duplicates.

So I would:

- Add "virtual void SymbolVendor::AddSymbols(Symtab *symtab);" to SymbolVendor 
that just calls through to its SymbolFile to do the work
- Add "virtual void SymbolFile::AddSymbols(Symtab *symtab)" to SymbolFile with 
default implementation that does nothing
- Override SymbolFile::AddSymbols() for SymbolFilePDB and add symbols to the 
provided symbol table
- Modify *SymbolVendor::GetSymtab()" to get the object file symbol table, then 
pass that along to any symbol file instances it owns to allow each symbol file 
to augment the symbol table
- Remove all "FindPublicSymbols()" code from patch
- Revert all symbol searching code to just use the Symtab class now that it 
contains all needed symbols


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53368



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

[Lldb-commits] [PATCH] D53436: [LLDB] - Implement the support for the .debug_loclists section.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Just fix the assert to use lldbassert so we don't crash as mentioned in the 
inline comment and this is good to go.




Comment at: source/Expression/DWARFExpression.cpp:3070
   // Not supported entry type
+  assert(false && "Not supported location list type");
   return false;

use lldbassert so this doesn't crash a release debugger.


https://reviews.llvm.org/D53436



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

[Lldb-commits] [PATCH] D52543: [DWARFSymbolFile] Add the module lock where necessary and assert that we own it.

[Greg Clayton via Phabricator via lldb-commits]

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

LGTM


https://reviews.llvm.org/D52543



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

[Lldb-commits] [PATCH] D53530: Fix (and improve) the support for C99 variable length array types

[Greg Clayton via Phabricator via lldb-commits]

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

Parsing a type shouldn't need an execution context and we shouldn't be 
re-parsing a type over and over for each frame. We should be encoding the array 
expression somewhere that we can access it when we need to get the number of 
children using the current execution context.

The way I would prefer to see this:

- Revert all SymbolFile changes that added an execution context to type parsing
- Change the type parsing logic in SymbolFileDWARF to store the array count 
expression in some format that is associated with the clang opaque type (clang 
type metadata maybe?).
- Change "virtual uint32_t TypeSystem::GetNumChildren(...);" and "virtual 
CompilerType TypeSystem::GetChildCompilerTypeAtIndex(...);" to take an 
execution context that defaults to nothing like you did with the type parsing 
in the first patch. This execution context can be used to evaluate the count 
expression as needed. We can attempt to grab the count expression from the 
clang opaque type that we stored in the above step, and if one exists, use the 
current frame to evaluate it correctly.






Comment at: 
source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp:213-229
+  // Handle variables with incomplete array types.
+  auto *type = in_value.GetCompilerType().GetOpaqueQualType();
+  auto qual_type = clang::QualType::getFromOpaquePtr(type).getCanonicalType();
+  if (qual_type->getTypeClass() == clang::Type::IncompleteArray) {
+if (auto variable = in_value.GetVariable()) {
+  auto *lldb_type = variable->GetType();
+  auto *symbol_file = lldb_type->GetSymbolFile();

We must abstract this via the TypeSystem stuff and make this happen via the 
CompilerType interface. What happens when "in_value" is a swift type or other 
type from the type system? Crash


https://reviews.llvm.org/D53530



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

[Lldb-commits] [PATCH] D53368: [Symbol] Search symbols with name and type in a symbol file

[Greg Clayton via Phabricator via lldb-commits]

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

Very close. Just a bit of cleanup now that we changed SymbolFilePDB where we 
don't seem to need m_public_symbols anymore. See inlined comments and let me 
know what you think




Comment at: include/lldb/lldb-forward.h:444
 StructuredDataPluginWP;
+typedef std::shared_ptr SymbolSP;
 typedef std::shared_ptr SymbolFileSP;

Do we need this anymore? See inlined comments below.



Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:1344-1346
+auto symbol_ptr = GetPublicSymbol(*pub_symbol);
+if (!symbol_ptr)
+  continue;

Maybe get the file address from "pub_symbol" and avoid converting to a SymbolSP 
that we will never use? See more comments below.



Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:1353
+
+symtab.AddSymbol(*symbol_ptr);
+  }

Just make  a local symbol and convert it from PDB to lldb_private::Symbol here? 
Something like:
```
symtab.AddSymbol(ConvertPDBSymbol(pdb_symbol));
```

So it seems we shouldn't need the m_public_symbols storage in this class 
anymore since "symtab" will now own the symbol we just created. 



Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:1968-1969
+
+lldb_private::Symbol *SymbolFilePDB::GetPublicSymbol(
+const llvm::pdb::PDBSymbolPublicSymbol &pub_symbol) {
+  auto it = m_public_symbols.find(pub_symbol.getSymIndexId());

Maybe convert this to:
```
lldb_private::Symbol ConvertPDBSymbol(const llvm::pdb::PDBSymbolPublicSymbol 
&pub_symbol)
```
And we only call this if we need to create a symbol we will add to the "symtab" 
in SymbolFilePDB::AddSymbols(...)



Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.h:247
   llvm::DenseMap m_variables;
+  llvm::DenseMap m_public_symbols;
   llvm::DenseMap m_public_names;

Do we need this mapping anymore? We should just add the symbols to the symbol 
table during SymbolFilePDB::AddSymbols(...).


https://reviews.llvm.org/D53368



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

[Lldb-commits] [PATCH] D53094: [pecoff] Implement ObjectFilePECOFF::GetDependedModules()

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Zach: yes the module mutex needs to be recursive. Plenty of places where the 
symbol file stuff can call other symbol file APIs. To avoid A/B locking issues, 
the lldb_private::Module, lldb_private::ObjectFile and 
lldb_private::SymbolVendor and lldb_private::SymbolFile all use the module 
mutex.


https://reviews.llvm.org/D53094



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

[Lldb-commits] [PATCH] D53597: Don't type-erase the SymbolContextItem enum

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

As long as Swig is happy and the ABI doesn't change I am ok with this. Will we 
see the variables better when debugging? Or is this solely so the 
SymbolContextItem type doesn't disappear from the debug info?


https://reviews.llvm.org/D53597



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

[Lldb-commits] [PATCH] D53368: [Symbol] Search symbols with name and type in a symbol file

[Greg Clayton via Phabricator via lldb-commits]

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

Very close, just down to making the SymbolVendor::GetSymtab() call 
symtab.CalculateSymbolSizes() and symtab.Finalize().




Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:1382-1383
+
+  symtab.CalculateSymbolSizes();
+  symtab.Finalize();
+}

Seems like these two lines should be done in the symbol vendor? Maybe this 
function should return the number of symbols added and the symbol vendor could 
see if AddSymbols returns a positive number, and if so, call 
symtab.CalculateSymbolSizes() and symtab.Finalize(). We should also see who 
else is calling these and remove any calls and only do it in the SymbolVendor 
one time.


https://reviews.llvm.org/D53368



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

[Lldb-commits] [PATCH] D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

The current SymbolFile::FindTypes(...) in was designed with type base name only 
due to how DWARF stores it variables. It has a "const CompilerDeclContext 
*parent_decl_ctx" which can be specified in order to limit what we find. So we 
might be able to think of this as a type lookup by basename. It might be handy 
to add another type lookup that does lookups by fully qualified name only. and 
leave the current infrastructure alone? A default implementation can be added 
that returns false for all symbol file unless they support name lookup. Also 
the current implementation allows you to under specify a type. For code like:

  struct Pt { char x; char y; };
  namespace a {
struct Pt { short x; short y; };
namespace b {
  struct Pt { int x; int y; };
}
  }

We can find all of them using:

  (lldb) type lookup Pt
  struct Pt {
  short x;
  short y;
  }
  struct Pt {
  int x;
  int y;
  }
  struct Pt {
  char x;
  char y;
  }
  
  Or each one individually using:
  
  (lldb) type lookup a::b::Pt
  struct Pt {
  int x;
  int y;
  }
  (lldb) type lookup a::Pt
  struct Pt {
  short x;
  short y;
  }
  (lldb) type lookup ::Pt
  struct Pt {
  char x;
  char y;
  }

Or under specify the namespace:

  (lldb) type lookup b::Pt
  struct Pt {
  int x;
  int y;
  }

That is the current behavior. Not saying it is right. Note we also don't 
display the decl context for a type when dumping it which would be nice to fix. 
So we probably need to make sure there are tests for all of the cases above 
once we hone in on the approach we want all symbol plug-ins to use.




Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2443
+
+  // FIXME: When exact_match is true, we should do an optimized O(1) lookup.
+  llvm::StringRef scope;

There is no O(1) lookup for types in DWARF. Accelerator tables do not have 
fully qualified type names, only the identifier for the type basename only.


https://reviews.llvm.org/D53662



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

[Lldb-commits] [PATCH] D53368: [Symbol] Search symbols with name and type in a symbol file

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In https://reviews.llvm.org/D53368#1276152, @aleksandr.urakov wrote:

> Yes, I'll implement it tomorrow (I'm already OOO now), thanks. But is it 
> really necessary to check the number of symbols added if we must to calculate 
> / finalize the symtab after getting it from object file anyway? May be just 
> always do it after creation and processing by the symbol file? For each 
> symtab it will be done just once because of caching.


yes, fine to still have void and always call symtab.CalculateSymbolSizes(); and 
symtab.Finalize() only in the symbol vendor. Find the other places this is 
called in the ObjectFile plug-ins and remove them and do them once in Symbol 
vendor when we fetch the symtab for the first time


https://reviews.llvm.org/D53368



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

[Lldb-commits] [PATCH] D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

My issue with adding "exact_match" is it renders a few of the arguments useless:

  size_t FindTypes(
const SymbolContext &sc, 
llvm::StringRef name,
const CompilerDeclContext *parent_decl_ctx, 
bool exact_match,
...);

With exact_match "parent_decl_ctx" is useless and possible the symbol context 
too. The FindTypes as written today was used for two cases which evolved over 
time:

- finding types for users without needing to be fully qualified
- finding types for the expression parser in a specific context

To fulfill these two cases, arguments have been added over time. As we keep 
adding arguments we make the function even harder to implement for SymbolFile 
subclasses. So as long as we are cleaning things up, it might be time to factor 
this out by making changes now.

Another complexity is that we can either search a single module or multiple 
modules when doing searches, and that is why we have 
Module::FindTypes_Impl(...) in since there is some work that needs to be done 
when we are searching by basename only (strip the context and search by 
basename, then filters matches afterward.

That is why I was thinking we might want to make changes. Seems like having:

  size_t FindTypesByBasename(
const SymbolContext &sc, 
llvm::StringRef name,
const CompilerDeclContext *parent_decl_ctx
...);
  
  size_t FindTypesByFullname(llvm::StringRef name, ...);

Clients of the first call must strip a typename to its identifier name prior to 
calling, and clients of the second can call with a fully qualified typename.


https://reviews.llvm.org/D53662



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

[Lldb-commits] [PATCH] D53506: [ClangASTContext] Extract VTable pointers from C++ objects

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Looks fine to me. Make sure no one else has any objections.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53506



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

[Lldb-commits] [PATCH] D53929: [LLDB] - Add support for DW_FORM_rnglistx and relative DW_RLE_* entries.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Just one question about extracting the value for DW_AT_ranges. It would be nice 
if we just took care of extracting the value so the form value was more useful




Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1072-1079
+  if (at_ranges_val != DW_INVALID_OFFSET) {
+if (DWARFDebugRangesBase *debug_ranges = dwarf2Data->DebugRanges()) {
+
+  dw_offset_t debug_ranges_offset;
+  if (form_value.Form() == DW_FORM_rnglistx)
+debug_ranges_offset = debug_ranges->GetOffset(at_ranges_val);
+  else

Can/should we do all this work when we extract the form value so that 
"form_value.Unsigned()" just returns the right thing? If not, every place that 
gets DW_AT_ranges attribute would need to do this.


https://reviews.llvm.org/D53929



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

[Lldb-commits] [PATCH] D53929: [LLDB] - Add support for DW_FORM_rnglistx and relative DW_RLE_* entries.

[Greg Clayton via Phabricator via lldb-commits]

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

Solution is fine. As long as we don't have to duplicate the work everywhere 
that needs a ranges offset.


https://reviews.llvm.org/D53929



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

[Lldb-commits] [PATCH] D53530: Fix (and improve) the support for C99 variable length array types

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

The only other thing you would need to change to get the usability back in 
check when doing things in GetNumChildren() would be to have the function that 
gets the typename take on optional execution context for dynamic types. The 
ValueObject can easily pass its execution context when getting the typename. 
Anyone who doesn't would continue to get the "int []" as the typename which 
isn't really lying either way. Thoughts?


https://reviews.llvm.org/D53530



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

[Lldb-commits] [PATCH] D53368: [Symbol] Search symbols with name and type in a symbol file

[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. Thanks for making the changes.


https://reviews.llvm.org/D53368



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

[Lldb-commits] [PATCH] D53530: Fix (and improve) the support for C99 variable length array types

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In https://reviews.llvm.org/D53530#1284267, @aprantl wrote:

> > I didn't realize that the string int [] is produced by ValueObject itself; 
> > I think this makes this option more palatable to me. I'll give it a try.
>
> So, it turns out it isn't. The only way to get the length into the typename 
> is to hack ClangASTContext::GetTypeName to replace the training "[]" of what 
> clang returns as the typename with a different string. The main problem with 
> this is that this will only work for outermost types.
>
> Something that has been requested in the past is to support C structs with 
> trailing array members, such as
>
>   struct variable_size {
> unsigned length;
> int __attribute__((size=.length)) elements[]; // I just made up this 
> attribute, but you get the basic idea.
>   };
>
>
> in a similar fashion. When printing such a struct, there's no way of safely 
> injecting the size into array type string any more.
>  If we dynamically created the correctly-sized array type instead, this would 
> work just fine.
>
> I haven't yet understood the motivation behind why overriding 
> GetNumChildren/GetTypeName/GetChildAtIndex is preferable over creating a 
> dynamic type in the language runtime. Is there something that I need to know?


Creating a new dynamic type every time you stop seems like a lot of work and a 
possible waste of memory. You will need to see if you already have such a type 
("int[4]" already exists) and use it if it does each time you stop. Any type 
that can change dynamically should easily be able to be detected by the type 
system in GetNumChildren(...) with the right execution context and and a 
dynamic type name can also easily be calculated in the type system. One option 
would be to just display "int[]" and have a summary for any dynamic array types 
that says "size = 4". Then we don't have to touch the typename.


https://reviews.llvm.org/D53530



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

[Lldb-commits] [PATCH] D51578: Contiguous .debug_info+.debug_types for D32167

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Just a few nits, but _very_ close.




Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h:65
   typedef std::vector CompileUnitColl;
+  typedef std::unordered_map TypeSignatureMap;
 

Use llvm::DenseMap here?



Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:797
+DWARFCompileUnit *DWARFUnit::GetAsCompileUnit() {
+  if (GetUnitDIEOnly().Tag() == DW_TAG_compile_unit)
+return static_cast(this);

Should this just be "if (GetUnitDIEOnly().Tag() != DW_TAG_type_unit)"? Partial 
units and many others can be top level DIEs.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D51578



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

[Lldb-commits] [PATCH] D53368: [Symbol] Search symbols with name and type in a symbol file

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

So it depends on what code was retrieving the symbol table from the object 
file. Can you detail where this was happening?


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53368



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

[Lldb-commits] [PATCH] D54003: Refactor ClangASTContext::AddEnumerationValueToEnumerationType() to remove redundant parameter which can be calculated from other parameter

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

LGTM. lldbassert to check that AST in "enum_type" is the same as "this" since 
we now can after switching to CompilerType as arg instead of opaque clang type 
pointer.




Comment at: include/lldb/Symbol/ClangASTContext.h:909
   clang::EnumConstantDecl *AddEnumerationValueToEnumerationType(
-  lldb::opaque_compiler_type_t type,
-  const CompilerType &enumerator_qual_type, const Declaration &decl,
-  const char *name, int64_t enum_value, uint32_t enum_value_bit_size);
+  const CompilerType enum_type, const Declaration &decl, const char *name,
+  int64_t enum_value, uint32_t enum_value_bit_size);

teemperor wrote:
> Can we pass `enum_type` as const ref?
CompilerType instances are two pointers. Pretty cheap to copy.



Comment at: source/Symbol/ClangASTContext.cpp:8851
 clang::EnumConstantDecl *ClangASTContext::AddEnumerationValueToEnumerationType(
-lldb::opaque_compiler_type_t type,
-const CompilerType &enumerator_clang_type, const Declaration &decl,
-const char *name, int64_t enum_value, uint32_t enum_value_bit_size) {
-  if (type && enumerator_clang_type.IsValid() && name && name[0]) {
-clang::QualType enum_qual_type(GetCanonicalQualType(type));
-
-bool is_signed = false;
-enumerator_clang_type.IsIntegerType(is_signed);
-const clang::Type *clang_type = enum_qual_type.getTypePtr();
-if (clang_type) {
-  const clang::EnumType *enutype =
-  llvm::dyn_cast(clang_type);
-
-  if (enutype) {
-llvm::APSInt enum_llvm_apsint(enum_value_bit_size, is_signed);
-enum_llvm_apsint = enum_value;
-clang::EnumConstantDecl *enumerator_decl =
-clang::EnumConstantDecl::Create(
-*getASTContext(), enutype->getDecl(), clang::SourceLocation(),
-name ? &getASTContext()->Idents.get(name)
- : nullptr, // Identifier
-ClangUtil::GetQualType(enumerator_clang_type),
-nullptr, enum_llvm_apsint);
-
-if (enumerator_decl) {
-  enutype->getDecl()->addDecl(enumerator_decl);
+const CompilerType enum_type, const Declaration &decl, const char *name,
+int64_t enum_value, uint32_t enum_value_bit_size) {

teemperor wrote:
> (Same as above) Can we have `enum_type` as a const ref?
CompilerType instances are two pointers. Pretty cheap to copy.



Comment at: source/Symbol/ClangASTContext.cpp:8853
+int64_t enum_value, uint32_t enum_value_bit_size) {
+
+  if (!enum_type || ConstString(name).IsEmpty())

lldbassert that the AST in "enum_type" is the same as "this"?


https://reviews.llvm.org/D54003



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

[Lldb-commits] [PATCH] D53530: Fix (and improve) the support for C99 variable length array types

[Greg Clayton via Phabricator via lldb-commits]

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

Thanks for making the changes!


https://reviews.llvm.org/D53530



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

[Lldb-commits] [PATCH] D54241: Fix bug in printing ValueObjects and in PE/COFF Plugin

[Greg Clayton via Phabricator via lldb-commits]

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

Very close, just use the layout type as mentioned in the inline comment.




Comment at: lldb/source/Core/ValueObjectVariable.cpp:70
   if (var_type)
-return var_type->GetForwardCompilerType();
+return var_type->GetFullCompilerType();
   return CompilerType();

Change this to GetLayoutCompilerType(). Why? If you have just a class Foo and 
your variable's type is "Foo *", then you don't need to complete it. The layout 
type gets you the type that is needed in order to display the current variable. 
For enums this will do the right thing. Otherwise if you have 1000 variables 
that are all pointers, you will be completing all of the class types for no 
reason.


https://reviews.llvm.org/D54241



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

[Lldb-commits] [PATCH] D54333: [CMake] Allow version overrides with -DLLDB_VERSION_MAJOR/MINOR/PATCH/SUFFIX

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

nice!


https://reviews.llvm.org/D54333



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

[Lldb-commits] [PATCH] D51578: Contiguous .debug_info+.debug_types for D32167

[Greg Clayton via Phabricator via lldb-commits]

clayborg accepted this revision.
clayborg added inline comments.
This revision is now accepted and ready to land.



Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h:65
   typedef std::vector CompileUnitColl;
+  typedef llvm::DenseMap TypeSignatureMap;
 

yeah, I kept getting people commenting on my patches that told me to use 
DenseMap and StringMap!


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D51578



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

[Lldb-commits] [PATCH] D54454: Be more permissive in what we consider a variable name.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Since we only handle . and -> this general idea seems ok to me. Do we even need 
a regex then? Maybe just search for first of ".-"?


https://reviews.llvm.org/D54454



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

[Lldb-commits] [PATCH] D54460: Don't keep a global ABI plugin per architecture

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Anything that takes a process shared pointer should be per process. A long time 
ago these plug-ins didn't take process, and as time went on they did take a 
process:

  306633   jmolenda   ABI(lldb::ProcessSP process_sp) {
  306633   jmolenda if (process_sp.get())
  306633   jmolenda m_process_wp = process_sp;
  306633   jmolenda   }
  245277 labath 
  306633   jmolenda   lldb::ProcessWP m_process_wp;
  306633   jmolenda 

The ABI plug-in started off just figuring out where return values were and and 
where argument values went.. If there was no process, then each ABI plug-in 
really is per architecture and they could be shared. Not sure if there is 
anything cached in any ABI plug-ins that is process specific. The change log 
says:

  Change the ABI class to have a weak pointer to its Process;
  some methods in the ABI need a Process to do their work.
  Instead of passing it in as a one-off argument to those
  methods, this patch puts it in the base class and the methods
  can retrieve if it needed.
  
  Note that ABI's are sometimes built without a Process 
  (e.g. SBTarget::GetStackRedZoneSize) so it's entirely
  possible that the process weak pointer will not be
  able to reconsistitue into a strong pointer.
  
   

So to avoid having to pass the process to ABI functions that require it, it 
sounds like Jason just put it into the class. We can either take the process 
out of the base class and pass it in where it is needed to each method and keep 
one copy, or we can make new ones for each process. They aren't huge.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54460



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

[Lldb-commits] [PATCH] D54454: Be more permissive in what we consider a variable name.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In https://reviews.llvm.org/D54454#1296392, @zturner wrote:

> BTW, I will have to see if it's possible to write a test for this.  Even when 
> I compiled and built a program with DWARF on Linux, the `target variable 
> Pi` example didn't "just work" for me, because `FindGlobalVariables` 
> wasn't returning the variable.  So I think this part actually needs to be 
> fixed in the DWARF plugin, which I'm not equipped to fix.  I can try 
> something that is not a variable template, such as the 
> `Foo::StaticMember` example, but if that also doesn't work, then there 
> might not be a good way to write a general purpose test for this until this 
> is fixed in the DWARF plugin.
>
> I can definitely add a test in the native pdb plugin though, and technically 
> that runs everywhere (although it would only test `target variable` and not 
> `frame variable`, maybe that's ok though?).


It is the template stuff that throws things off. Can't remember if there is an 
accelerator table entry for those. Global variables in general work. It would 
be good to figure out what this doesn't work for DWARF. I will take a look.


https://reviews.llvm.org/D54454



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

[Lldb-commits] [PATCH] D52543: [DWARFSymbolFile] Add the module lock where necessary and assert that we own it.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

yep


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52543



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

[Lldb-commits] [PATCH] D54863: [ASTImporter] Set MustBuildLookupTable on PrimaryContext

[Greg Clayton via Phabricator via lldb-commits]

clayborg accepted this revision.
clayborg added a comment.

Looks good to me as long as test suite is happy.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D54863



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

[Lldb-commits] [PATCH] D54843: [Expr] Check the language before ignoring Objective C keywords

[Greg Clayton via Phabricator via lldb-commits]

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



Comment at: source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp:404
-// non-Apple platforms, but for now it is needed.
-m_compiler->getLangOpts().ObjC = true;
 break;

A better way would be to try and grab the Objective C runtime from the process. 
There are some variants of objective C that might run under non-apple targets:

```
ProcessSP process_sp = target->GetProcess();
if (process_sp) 
  m_compiler->getLangOpts().ObjC = 
process_sp->GetLanguageRuntime(eLanguageTypeObjC) != nullptr;
```

Then C and C++ programs on Mac will be able to use "id" and other reserved 
words in their expressions again.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D54843



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

[Lldb-commits] [PATCH] D54751: [LLDB] - Fix setting the breakpoints when -gsplit-dwarf and DWARF 5 were used for building the executable.

[Greg Clayton via Phabricator via lldb-commits]

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

See inlined comments.




Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:353
+  // unit in contrast.
+  if (!addr_base)
+addr_base = cu_die.GetAttributeValueAsUnsigned(m_dwarf, this,

```
if (addr_base == LLDB_INVALID_ADDRESS)
```


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

https://reviews.llvm.org/D54751



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

[Lldb-commits] [PATCH] D54751: [LLDB] - Fix setting the breakpoints when -gsplit-dwarf and DWARF 5 were used for building the executable.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:354-355
+  if (!addr_base)
+addr_base = cu_die.GetAttributeValueAsUnsigned(m_dwarf, this,
+   DW_AT_GNU_addr_base, 0);
   dwo_cu->SetAddrBase(addr_base);

```
addr_base = cu_die.GetAttributeValueAsUnsigned(m_dwarf, this, 
DW_AT_GNU_addr_base, LLDB_INVALID_ADDRESS);
```


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

https://reviews.llvm.org/D54751



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

[Lldb-commits] [PATCH] D54751: [LLDB] - Fix setting the breakpoints when -gsplit-dwarf and DWARF 5 were used for building the executable.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Missed an inline comment on last comment




Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:311
+  dw_addr_t addr_base =
+  cu_die.GetAttributeValueAsUnsigned(m_dwarf, this, DW_AT_addr_base, 
0);
+  SetAddrBase(addr_base);

Use LLDB_INVALID_ADDRESS instead of zero as zero could be a valid base address.
```
dw_addr_t addr_base = cu_die.GetAttributeValueAsUnsigned(m_dwarf, this, 
DW_AT_addr_base, LLDB_INVALID_ADDRESS);
```


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

https://reviews.llvm.org/D54751



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

[Lldb-commits] [PATCH] D28305: [Host] Handle short reads and writes, take 3

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Any reason we are removing File::SeekFromCurrent and File::SeekFromEnd?


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

https://reviews.llvm.org/D28305



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

[Lldb-commits] [PATCH] D54692: [Driver] Use libOption with tablegen.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Would be great to see old and new output like Zach suggested. Is there a reason 
we need to use TableGen? Other command line tools just use llvm:🆑:opt stuff. 
Seems a bit obtuse to use TableGen?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D54692



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

[Lldb-commits] [PATCH] D54692: [Driver] Use libOption with tablegen.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

the old usage:

  Usage:
  
lldb -h
lldb -v [[--]  [ ...]]
lldb -a  -f  [-c ] [-s ] [-o ] 
[-S ] [-O ] [-k ] [-K ] [-Q] [-b] [-e] [-x] 
[-X] [-l ] [-d] [-z ] [[--]  
[ ...]]
lldb -n  -w [-s ] [-o ] [-S ] [-O 
] [-k ] [-K ] [-Q] [-b] [-e] [-x] [-X] [-l 
] [-d] [-z ]
lldb -p  [-s ] [-o ] [-S ] [-O ] [-k 
] [-K ] [-Q] [-b] [-e] [-x] [-X] [-l ] [-d] 
[-z ]
lldb -P
lldb -r [] -R 

Showed how all the options can be used together. It would be nice to group the 
options into the following groups:

- general options

--arch, --version, --help, --file, --editor)

- LLDB command options (--batch, --one-line*, --source*
- attach options (--attach-name, --attach-pid, --wait-for


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D54692



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

[Lldb-commits] [PATCH] D54692: [Driver] Use libOption with tablegen.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Can we still group the options as mentioned in my previous comment?




Comment at: tools/driver/Driver.cpp:943
+usage << '\n';
+usage << argv[0] << " -h" << '\n';
+usage << argv[0] << " -v [[--]  [ ...]]\n";

Get file base name of this so we don't show a full path to the tool if the user 
used the full path?


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

https://reviews.llvm.org/D54692



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

[Lldb-commits] [PATCH] D54692: [Driver] Use libOption with tablegen.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

I know cl::opt had ways to group options and the table gen is more powerful so 
it must have this feature?


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

https://reviews.llvm.org/D54692



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

[Lldb-commits] [PATCH] D54692: [Driver] Use libOption with tablegen.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Can you attach new output with the grouping and extra usage?


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

https://reviews.llvm.org/D54692



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

[Lldb-commits] [PATCH] D54692: [Driver] Use libOption with tablegen.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

I would wait to get another OK from anyone else just to be sure.


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

https://reviews.llvm.org/D54692



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

[Lldb-commits] [PATCH] D54692: [Driver] Use libOption with tablegen.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

I would be happy if we just have an EXAMPLES section much like many of the man 
pages for built in shell commands where we show a few examples.

I would like to see setting up a target a program with arguments that might 
conflict with the argument parser like:

  % lldb --arch x86_64 /path/to/program/a.out -- --arch arvm7

The first arch is for lldb, and any options past the -- are for the inferior 
program

Attaching examples, loading a core file, printing python path, maybe specifying 
some lldb commands with -o and -O to show how those work.


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

https://reviews.llvm.org/D54692



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

[Lldb-commits] [PATCH] D54751: [LLDB] - Fix setting the breakpoints when -gsplit-dwarf and DWARF 5 were used for building the executable.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:355-356
+  if (addr_base == LLDB_INVALID_ADDRESS)
+addr_base = cu_die.GetAttributeValueAsUnsigned(m_dwarf, this,
+   DW_AT_GNU_addr_base, 0);
   dwo_cu->SetAddrBase(addr_base);

Do we still want the addr_base to default to zero instead of 
LLDB_INVALID_ADDRESS here?


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

https://reviews.llvm.org/D54751



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

[Lldb-commits] [PATCH] D54751: [LLDB] - Fix setting the breakpoints when -gsplit-dwarf and DWARF 5 were used for building the executable.

[Greg Clayton via Phabricator via lldb-commits]

clayborg accepted this revision.
clayborg added inline comments.
This revision is now accepted and ready to land.



Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:355-356
+  if (addr_base == LLDB_INVALID_ADDRESS)
+addr_base = cu_die.GetAttributeValueAsUnsigned(m_dwarf, this,
+   DW_AT_GNU_addr_base, 0);
   dwo_cu->SetAddrBase(addr_base);

grimar wrote:
> clayborg wrote:
> > Do we still want the addr_base to default to zero instead of 
> > LLDB_INVALID_ADDRESS here?
> I believe so. Setting the addr_base to default zero looks fine to me. That 
> way the existent code does not
> need to check for LLDB_INVALID_ADDRESS and can just call the `GetAddrBase` 
> and add the result
> to the offset calculation.
> That is how things already work and I do not see any benefits from changing 
> this behavior probably?
Sounds good, just wanted to check.


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

https://reviews.llvm.org/D54751



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

[Lldb-commits] [PATCH] D48752: Quiet command regex instructions during batch execution

[Greg Clayton via Phabricator via lldb-commits]

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

Seems like we should just add a "bool interactive" as a second parameter to 
"IOHandlerActivated". Then it will be easy to find the other places that need 
to be fixed up.




Comment at: include/lldb/Core/IOHandler.h:201
 
   virtual void IOHandlerActivated(IOHandler &io_handler) {}
 

Maybe remove the function below and add a "bool interactive" as a second 
parameter to this function?



Comment at: source/Commands/CommandObjectCommands.cpp:983
 protected:
-  void IOHandlerActivated(IOHandler &io_handler) override {
+  void IOHandlerActivatedInteractively(IOHandler &io_handler) override {
 StreamFileSP output_sp(io_handler.GetOutputStreamFile());

See above inline comment about leaving the name the same but adding the "bool 
interactive" as a paramter



Comment at: source/Commands/CommandObjectCommands.cpp:985
 StreamFileSP output_sp(io_handler.GetOutputStreamFile());
 if (output_sp) {
+  output_sp->PutCString("Enter one or more sed substitution commands in "

If we make changes I requested above this would become:
```
if (output_sp && interactive)
```



Comment at: source/Core/IOHandler.cpp:335-337
   m_delegate.IOHandlerActivated(*this);
+  if (GetIsInteractive())
+m_delegate.IOHandlerActivatedInteractively(*this);

```
m_delegate. IOHandlerActivated(*this, GetIsInteractive());
```


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

https://reviews.llvm.org/D48752



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

[Lldb-commits] [PATCH] D54843: [Expr] Check the language before ignoring Objective C keywords

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Looks fine to me. Jim Ingham should ok this as well.


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

https://reviews.llvm.org/D54843



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

[Lldb-commits] [PATCH] D55214: Introduce ObjectFileBreakpad

[Greg Clayton via Phabricator via lldb-commits]

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

This looks like a good start.


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

https://reviews.llvm.org/D55214



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

[Lldb-commits] [PATCH] D55230: [lit] Multiple build outputs and default target bitness

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

What is the reason we aren't using cmake + ninja for this kind of stuff? Or is 
it using it under the covers?


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

https://reviews.llvm.org/D55230



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

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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

I like seeing all of the cmake modifications for the LLDB.framework. Are we 
planning on trying to get rid of the Xcode project at some point soon and use 
the auto generated one made by cmake?


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

https://reviews.llvm.org/D55328



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

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

[Greg Clayton via Phabricator via lldb-commits]

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

This is already available with:

  virtual lldb_private::Address ObjectFile::GetHeaderAddress(); 

It return a lldb_private::Address which is section offset, but nothing stopping 
us from returning a lldb_private::Address with no section and just the file 
address. For mach-o the mach header is in the __TEXT segment, but not true for 
other file formats. I am ok if we need to rename "GetHeaderAddress()" to 
something else.


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.

Marked as requesting changes in case "GetBaseFileAddress() == 
GetHeaderAddress().GetFileAddress()" in all cases. If the base file address 
differs from where the object file header is located, then this change would 
work. Else we should use GetHeaderAddress() and possibly rename it if we want to


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 inline comments.



Comment at: include/lldb/Symbol/ObjectFile.h:569
 
+  /// Returns the base file address of an object file (also known as the
+  /// preferred load address or image base address). This is typically the file

lemo wrote:
> "file address" can mean an offset within a container file.
> 
> to avoid any confusion I'd use "base address" (or "image base address")
"file address" is the correct LLDB term so I would leave this as is. File 
addresses in LLDB are addresses as they are represented within the object file 
itself. Load addresses at unique and map to a single address in a live process 
where shared libraries are slid around. So "file address" is the correct LLDB 
term.


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] D55422: Rename ObjectFile::GetHeaderAddress to GetBaseAddress

[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 fine. Just one clarification in the header documentation and this is good 
to go.




Comment at: include/lldb/Symbol/ObjectFile.h:561
+  /// will have this section set. Otherwise, the address will just have the
+  /// offset member filled in.
   //--

```
/// offset member filled in indicating the resulting Address object represents 
a file address.
```



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

Switching to IsInMemory will work. This code probably predates the existence of 
IsInMemory().


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

[Lldb-commits] [PATCH] D55434: ObjectFileBreakpad: Implement sections

[Greg Clayton via Phabricator via lldb-commits]

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

So sections should correspond to different kinds of sections, like .text, 
.data, etc. If we have the following breakpad file:

  MODULE Linux x86_64 24B5D199F0F766FF5DC30 linux.out
  INFO CODE_ID B52499D1F0F766FF5DC3
  FILE 0 /tmp/a.c
  FUNC 1010 10 0 _start
  1010 4 4 0
  1014 5 5 0
  1019 5 6 0
  101e 2 7 0
  FUNC 2010 10 0 main
  2010 4 4 0
  2014 5 5 0
  2019 5 6 0
  201e 2 7 0
  PUBLIC 1010 0 _start
  PUBLIC 2010 0 main
  PUBLIC 3010 0 nodebuginfo1
  PUBLIC 3020 0 nodebuginfo2

I would expect to have just 1 section named ".text" with read and execute 
permissions. This section would have its m_file_addr set to 0x1010 (from FUNC 
or PUBLIC with lowest address ("FUNC 1010 10 0 _start" in this case)). The size 
of the section would be set to the max code address + size minus the lowest 
code address (0x3020 - 0x1010 in this case). The file offset and file size 
should be set to zero since section contents are typically the bytes for the 
disassembly.

One other way to do this would be to create a section for each FUNC whose 
m_file_addr it set to the FUNC start address, and whose size is set to the last 
line entry - FUNC start address. Then name of the section can be set to the 
function name in this case. I am not a huge fan of this since it just creates 
extra sections for no reason and the debug info will have this info anyway so 
it will be duplicated.

I see no reason to create sections for MODULE, INFO, FILE, or STACK records. 
Was there a reason you wanted to create sections for all these? Might be better 
to create symbols for any of these you need to reference later.

Once you start parsing the debug info, the lldb::user_id_t for any items, like 
FUNC, can just be the line number or character offset for the FUNC source line 
within the file.


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

https://reviews.llvm.org/D55434



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

[Lldb-commits] [PATCH] D55434: ObjectFileBreakpad: Implement sections

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Another point of clarification is that sections exist in order to lookup 
addresses and resolve addresses to a section within a file. The section should 
be something that can easily be slid around when loaded by LLDB when we are 
debugging or symbolicating. So any sections we create should be able to be have 
the section load address set in the target with code like:

  if (target.GetSectionLoadList().SetSectionLoadAddress(section_sp, 
section_sp->GetFileAddress() + slide))

All of the sections you added, except the FUNC section, wouldn't end up ever 
being loaded. All items besides FUNC might be better represented as symbols.


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

https://reviews.llvm.org/D55434



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

[Lldb-commits] [PATCH] D55434: ObjectFileBreakpad: Implement sections

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

So we do something like you describe with the DYSM files. The object file is 
"a.out" and it has a dSYM file "a.out.dSYM/Context/Resources/DWARF/a.out" and 
the dSYM file will share sections with the "a.out" object file. So if you plan 
on loading the breakpad file as a symbol file that just needs some sections 
that it can give to the debug info it will eventually create, these sections 
must be the same between the binary and the breakpad object file. So I would 
still recommend trying to make sections that make sense like a real object 
file. It is also nice to be able to live off of the breakpad file itself. In 
this case the ObjectFile for breakpad when it is stand alone should do as good 
of a job as possible.


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

https://reviews.llvm.org/D55434



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

[Lldb-commits] [PATCH] D55434: ObjectFileBreakpad: Implement sections

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

If you plan on not making the breakpad file ever stand alone, then you will 
need to take any addresses and look them up in the module section list and use 
the other sections. I don't see why the breakpad file can't be stand alone 
though. It won't be as accurate, but it sure would be nice to be able to load a 
bunch of them into LLDB without needing to find the original executable and 
just symbolicate no?


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

https://reviews.llvm.org/D55434



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

[Lldb-commits] [PATCH] D55472: Speadup memory regions handling and cleanup related code

[Greg Clayton via Phabricator via lldb-commits]

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

I am going to stop making comments as I have been working on a similar patch 
that does more than this one. I will post it today and we can decide which 
approach we want to use.




Comment at: include/lldb/API/SBMemoryRegionInfoList.h:29
 
-  uint32_t GetSize() const;
+  size_t GetSize() const;
 

Revert. You can add functions, but never change them. Our API is stable. 



Comment at: include/lldb/API/SBMemoryRegionInfoList.h:31
 
-  bool GetMemoryRegionAtIndex(uint32_t idx, SBMemoryRegionInfo ®ion_info);
+  void Reserve(size_t capacity);
 

This you can add, but std::vector implementations are quite efficient and I 
doubt this is really needed?



Comment at: include/lldb/API/SBMemoryRegionInfoList.h:33
 
-  void Append(lldb::SBMemoryRegionInfo ®ion);
+  bool GetMemoryRegionAtIndex(size_t idx, SBMemoryRegionInfo ®ion_info);
 

Revert to maintain API



Comment at: include/lldb/API/SBMemoryRegionInfoList.h:35
 
-  void Append(lldb::SBMemoryRegionInfoList ®ion_list);
+  void Append(const lldb::SBMemoryRegionInfo ®ion);
+

"const" means nothing to our objects as they have shared pointers or unique 
pointers which never change. 



Comment at: include/lldb/API/SBMemoryRegionInfoList.h:37
+
+  void Append(const lldb::SBMemoryRegionInfoList ®ion_list);
 

ditto, and revert for API



Comment at: include/lldb/Target/Process.h:2084
   virtual Status
-  GetMemoryRegions(std::vector ®ion_list);
+  GetMemoryRegions(std::vector ®ion_list);
 

This was a a vector of shared pointers, seemingly for no good reason prior to 
this change, I would change this to be a vector of MemoryRegionInfo instances? 



Comment at: source/API/SBMemoryRegionInfo.cpp:23-24
 
-SBMemoryRegionInfo::SBMemoryRegionInfo(const MemoryRegionInfo *lldb_object_ptr)
-: m_opaque_ap(new MemoryRegionInfo()) {
-  if (lldb_object_ptr)
-ref() = *lldb_object_ptr;
-}
+SBMemoryRegionInfo::SBMemoryRegionInfo(MemoryRegionInfoUP &&lldb_object_up)
+: m_opaque_ap(std::move(lldb_object_up)) {}
 

clayborg wrote:
> Don't remove... API
Fine to add to API, but really no one outside LLDB will be able to do anything 
with this function (nor the one on the left) since MemoryRegionInfo is opaque 
as far as the public clients are concerned. It is there for internal LLDB 
convenience. So do we really need this? We can add a method that copies from an 
instance if needed. MemoryRegionInfo structs are very simple to copy. No need 
for heroics here.



Comment at: source/API/SBMemoryRegionInfo.cpp:23-27
-SBMemoryRegionInfo::SBMemoryRegionInfo(const MemoryRegionInfo *lldb_object_ptr)
-: m_opaque_ap(new MemoryRegionInfo()) {
-  if (lldb_object_ptr)
-ref() = *lldb_object_ptr;
-}

Don't remove... API



Comment at: source/API/SBMemoryRegionInfo.cpp:39-44
+SBMemoryRegionInfo &SBMemoryRegionInfo::
+operator=(MemoryRegionInfoUP &&lldb_object_up) {
+  m_opaque_ap = std::move(lldb_object_up);
+  return *this;
+}
+

Do this by instance instead of a move assignment operator with a unique ptr?



Comment at: source/API/SBMemoryRegionInfoList.cpp:81
 
-uint32_t SBMemoryRegionInfoList::GetSize() const {
+size_t SBMemoryRegionInfoList::GetSize() const {
   return m_opaque_ap->GetSize();

revert



Comment at: source/API/SBMemoryRegionInfoList.cpp:85-88
+void SBMemoryRegionInfoList::Reserve(size_t capacity) {
+  m_opaque_ap->Reserve(capacity);
+}
+

Who needs this externally? I would rather not add this to the public API? 



Comment at: source/API/SBMemoryRegionInfoList.cpp:90
 bool SBMemoryRegionInfoList::GetMemoryRegionAtIndex(
-uint32_t idx, SBMemoryRegionInfo ®ion_info) {
+size_t idx, SBMemoryRegionInfo ®ion_info) {
   Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));

revert



Comment at: source/API/SBMemoryRegionInfoList.cpp:110
 
-void SBMemoryRegionInfoList::Append(SBMemoryRegionInfo &sb_region) {
+void SBMemoryRegionInfoList::Append(const SBMemoryRegionInfo &sb_region) {
   m_opaque_ap->Append(sb_region);

remove const



Comment at: source/API/SBMemoryRegionInfoList.cpp:115
+void
+SBMemoryRegionInfoList::Append(const SBMemoryRegionInfoList &sb_region_list) {
   m_opaque_ap->Append(*sb_region_list);

revert



Comment at: source/API/SBProcess.cpp:1367
+
+  MemoryRegionInfoUP region_info_up(new lldb_private::MemoryRegionInfo());
   sb_error.ref() =

Remove. "sb_region_info" already has an instance we can use.



Comment at: source/API/SBP

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

[Greg Clayton via Phabricator via lldb-commits]

clayborg created this revision.
clayborg added reviewers: labath, markmentovai, zturner, tatyana-krasnukha.
Herald added a subscriber: mgrang.

Breakpad creates minidump files that sometimes have:

- linux maps textual content
- no MemoryInfoList

Right now unless the file has a MemoryInfoList we get no region information.

This patch:

- reads and caches the memory region info one time and sorts it for easy 
subsequent access
- get the region info from the best source in this order:
  - linux maps info (if available)
  - MemoryInfoList (if available)
  - MemoryList or Memory64List
- returns memory region info for the gaps between regions (before the first and 
after the last)

This patch is a different patch that would replace: 
https://reviews.llvm.org/D55472

If we decide to go with this patch, then I will add tests.


https://reviews.llvm.org/D55522

Files:
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h

Index: source/Plugins/Process/minidump/MinidumpParser.h
===
--- source/Plugins/Process/minidump/MinidumpParser.h
+++ source/Plugins/Process/minidump/MinidumpParser.h
@@ -86,7 +86,7 @@
 
   llvm::ArrayRef GetMemory(lldb::addr_t addr, size_t size);
 
-  llvm::Optional GetMemoryRegionInfo(lldb::addr_t);
+  llvm::Optional GetMemoryRegionInfo(lldb::addr_t load_addr);
 
   // Perform consistency checks and initialize internal data structures
   Status Initialize();
@@ -94,10 +94,18 @@
 private:
   MinidumpParser(const lldb::DataBufferSP &data_buf_sp);
 
+  bool CreateRegionsCacheFromLinuxMaps();
+  bool CreateRegionsCacheFromMemoryInfoList();
+  bool CreateRegionsCacheFromMemoryList();
+  bool CreateRegionsCacheFromMemory64List();
+  llvm::Optional
+  FindMemoryRegion(lldb::addr_t load_addr) const;
+
 private:
   lldb::DataBufferSP m_data_sp;
   llvm::DenseMap m_directory_map;
   ArchSpec m_arch;
+  std::vector m_regions;
 };
 
 } // end namespace minidump
Index: source/Plugins/Process/minidump/MinidumpParser.cpp
===
--- source/Plugins/Process/minidump/MinidumpParser.cpp
+++ source/Plugins/Process/minidump/MinidumpParser.cpp
@@ -19,6 +19,7 @@
 #include 
 #include 
 #include 
+#include 
 
 using namespace lldb_private;
 using namespace minidump;
@@ -401,72 +402,207 @@
   return range->range_ref.slice(offset, overlap);
 }
 
-llvm::Optional
-MinidumpParser::GetMemoryRegionInfo(lldb::addr_t load_addr) {
-  MemoryRegionInfo info;
+bool MinidumpParser::CreateRegionsCacheFromLinuxMaps() {
+  llvm::ArrayRef data = GetStream(MinidumpStreamType::LinuxMaps);
+  if (data.empty())
+return false;
+  llvm::StringRef text((const char *)data.data(), data.size());
+  llvm::StringRef line;
+  constexpr const auto yes = MemoryRegionInfo::eYes;
+  constexpr const auto no = MemoryRegionInfo::eNo;
+  while (!text.empty()) {
+std::tie(line, text) = text.split('\n');
+// Parse the linux maps line. Example line is:
+// 400b3000-400b5000 r-xp  b3:17 159/system/bin/app_process
+uint64_t start_addr, end_addr, offset;
+uint32_t device_major, device_minor, inode;
+if (line.consumeInteger(16, start_addr))
+  continue;
+if (!line.consume_front("-"))
+  continue;
+if (line.consumeInteger(16, end_addr))
+  continue;
+line = line.ltrim();
+llvm::StringRef permissions = line.substr(0, 4);
+line = line.drop_front(4);
+line = line.ltrim();
+if (line.consumeInteger(16, offset))
+  continue;
+line = line.ltrim();
+if (line.consumeInteger(16, device_major))
+  continue;
+if (!line.consume_front(":"))
+  continue;
+if (line.consumeInteger(16, device_minor))
+  continue;
+line = line.ltrim();
+if (line.consumeInteger(16, inode))
+  continue;
+line = line.ltrim();
+llvm::StringRef pathname = line;
+MemoryRegionInfo region;
+region.GetRange().SetRangeBase(start_addr);
+region.GetRange().SetRangeEnd(end_addr);
+region.SetName(pathname.str().c_str());
+region.SetReadable(permissions[0] == 'r' ? yes : no);
+region.SetWritable(permissions[1] == 'w' ? yes : no);
+region.SetExecutable(permissions[2] == 'x' ? yes : no);
+region.SetMapped(yes);
+m_regions.push_back(region);
+  }
+  return !m_regions.empty();
+}
+
+bool MinidumpParser::CreateRegionsCacheFromMemoryInfoList() {
   llvm::ArrayRef data = GetStream(MinidumpStreamType::MemoryInfoList);
   if (data.empty())
-return llvm::None;
-
+return false;
   std::vector mem_info_list =
   MinidumpMemoryInfo::ParseMemoryInfoList(data);
   if (mem_info_list.empty())
-return llvm::None;
-
-  const auto yes = MemoryRegionInfo::eYes;
-  const auto no = MemoryRegionInfo::eNo;
-
-  const MinidumpMemoryInfo *next_entry = nullptr;
-  for (const auto &entry : mem_info_list) {
-const auto head = entry->base_address;
-const auto tail = head + entry

[Lldb-commits] [PATCH] D55472: Speadup memory regions handling and cleanup related code

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

See my patch: https://reviews.llvm.org/D55522

Let me know what you think


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55472



___
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 updated this revision to Diff 177555.
clayborg added a comment.

Add sorting functions needed for MemoryRegionInfo


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

https://reviews.llvm.org/D55522

Files:
  include/lldb/Target/MemoryRegionInfo.h
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h

Index: source/Plugins/Process/minidump/MinidumpParser.h
===
--- source/Plugins/Process/minidump/MinidumpParser.h
+++ source/Plugins/Process/minidump/MinidumpParser.h
@@ -86,7 +86,7 @@
 
   llvm::ArrayRef GetMemory(lldb::addr_t addr, size_t size);
 
-  llvm::Optional GetMemoryRegionInfo(lldb::addr_t);
+  llvm::Optional GetMemoryRegionInfo(lldb::addr_t load_addr);
 
   // Perform consistency checks and initialize internal data structures
   Status Initialize();
@@ -94,10 +94,18 @@
 private:
   MinidumpParser(const lldb::DataBufferSP &data_buf_sp);
 
+  bool CreateRegionsCacheFromLinuxMaps();
+  bool CreateRegionsCacheFromMemoryInfoList();
+  bool CreateRegionsCacheFromMemoryList();
+  bool CreateRegionsCacheFromMemory64List();
+  llvm::Optional
+  FindMemoryRegion(lldb::addr_t load_addr) const;
+
 private:
   lldb::DataBufferSP m_data_sp;
   llvm::DenseMap m_directory_map;
   ArchSpec m_arch;
+  std::vector m_regions;
 };
 
 } // end namespace minidump
Index: source/Plugins/Process/minidump/MinidumpParser.cpp
===
--- source/Plugins/Process/minidump/MinidumpParser.cpp
+++ source/Plugins/Process/minidump/MinidumpParser.cpp
@@ -19,6 +19,7 @@
 #include 
 #include 
 #include 
+#include 
 
 using namespace lldb_private;
 using namespace minidump;
@@ -401,72 +402,207 @@
   return range->range_ref.slice(offset, overlap);
 }
 
-llvm::Optional
-MinidumpParser::GetMemoryRegionInfo(lldb::addr_t load_addr) {
-  MemoryRegionInfo info;
+bool MinidumpParser::CreateRegionsCacheFromLinuxMaps() {
+  llvm::ArrayRef data = GetStream(MinidumpStreamType::LinuxMaps);
+  if (data.empty())
+return false;
+  llvm::StringRef text((const char *)data.data(), data.size());
+  llvm::StringRef line;
+  constexpr const auto yes = MemoryRegionInfo::eYes;
+  constexpr const auto no = MemoryRegionInfo::eNo;
+  while (!text.empty()) {
+std::tie(line, text) = text.split('\n');
+// Parse the linux maps line. Example line is:
+// 400b3000-400b5000 r-xp  b3:17 159/system/bin/app_process
+uint64_t start_addr, end_addr, offset;
+uint32_t device_major, device_minor, inode;
+if (line.consumeInteger(16, start_addr))
+  continue;
+if (!line.consume_front("-"))
+  continue;
+if (line.consumeInteger(16, end_addr))
+  continue;
+line = line.ltrim();
+llvm::StringRef permissions = line.substr(0, 4);
+line = line.drop_front(4);
+line = line.ltrim();
+if (line.consumeInteger(16, offset))
+  continue;
+line = line.ltrim();
+if (line.consumeInteger(16, device_major))
+  continue;
+if (!line.consume_front(":"))
+  continue;
+if (line.consumeInteger(16, device_minor))
+  continue;
+line = line.ltrim();
+if (line.consumeInteger(16, inode))
+  continue;
+line = line.ltrim();
+llvm::StringRef pathname = line;
+MemoryRegionInfo region;
+region.GetRange().SetRangeBase(start_addr);
+region.GetRange().SetRangeEnd(end_addr);
+region.SetName(pathname.str().c_str());
+region.SetReadable(permissions[0] == 'r' ? yes : no);
+region.SetWritable(permissions[1] == 'w' ? yes : no);
+region.SetExecutable(permissions[2] == 'x' ? yes : no);
+region.SetMapped(yes);
+m_regions.push_back(region);
+  }
+  return !m_regions.empty();
+}
+
+bool MinidumpParser::CreateRegionsCacheFromMemoryInfoList() {
   llvm::ArrayRef data = GetStream(MinidumpStreamType::MemoryInfoList);
   if (data.empty())
-return llvm::None;
-
+return false;
   std::vector mem_info_list =
   MinidumpMemoryInfo::ParseMemoryInfoList(data);
   if (mem_info_list.empty())
-return llvm::None;
-
-  const auto yes = MemoryRegionInfo::eYes;
-  const auto no = MemoryRegionInfo::eNo;
-
-  const MinidumpMemoryInfo *next_entry = nullptr;
-  for (const auto &entry : mem_info_list) {
-const auto head = entry->base_address;
-const auto tail = head + entry->region_size;
-
-if (head <= load_addr && load_addr < tail) {
-  info.GetRange().SetRangeBase(
-  (entry->state != uint32_t(MinidumpMemoryInfoState::MemFree))
-  ? head
-  : load_addr);
-  info.GetRange().SetRangeEnd(tail);
-
+return false;
+  
+  constexpr const auto yes = MemoryRegionInfo::eYes;
+  constexpr const auto no = MemoryRegionInfo::eNo;
   const uint32_t PageNoAccess =
   static_cast(MinidumpMemoryProtectionContants::PageNoAccess);
-  info.SetReadable((entry->protect & PageNoAccess) == 0 ? yes : no);
-
   const uin

[Lldb-commits] [PATCH] D55434: ObjectFileBreakpad: Implement sections

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D55434#1325739 , @labath wrote:

> In D55434#1323912 , @clayborg wrote:
>
> > If you plan on not making the breakpad file ever stand alone, then you will 
> > need to take any addresses and look them up in the module section list and 
> > use the other sections. I don't see why the breakpad file can't be stand 
> > alone though. It won't be as accurate, but it sure would be nice to be able 
> > to load a bunch of them into LLDB without needing to find the original 
> > executable and just symbolicate no?
>
>
> I could try to make it stand-alone, but that seems to me like a duplication 
> of effort. And since the sections I could conjure up from the breakpad info 
> would never match the original elf file, I would have to support both cases 
> anyway, one with using the sections from the object file, and one with the 
> own, made-up sections.
>
> I do intend to support both cases, but in a slightly different way. The way, 
> I see it we have actually four cases to consider:
>
> 1. we have an stripped elf file, and a breakpad symbol file (the case of an 
> unstripped elf file is uninteresting, as it will have much better debug info 
> than breakpad can possibly provide)
> 2. we don't have an elf file, but we have a breakpad file
> 3. we don't have an elf file nor a breakpad file
>
>   Because of case 3, we have to do some section conjuring independently of 
> any breakpad file. We already do that to some extent, and @lemo is getting 
> ready to extend that in D55142 . Once we 
> have that, and we find a breakpad file for this module (case 2), the breakpad 
> file should be able to just latch onto the sections created for the 
> placeholder object file. And I believe ProcessMinidump is in a better 
> position to create the "placeholder" sections, as it has both access to the 
> load addresses and sizes of the modules. From breakpad info, it would be hard 
> to determine the size of the module if the highest address is occupied by a 
> "PUBLIC" symbol, as they don't have any sizes associated with them.
>
>   Going back to case 1, if we have a stripped elf file, the breakpad file 
> should latch onto the sections of that one without ever knowing the 
> difference. So in the end, I hope this will produce a clearer code because 
> the concerns will be separated. Breakpad code will always deal with 
> externally-provided sections, regardless of whether they come from a "real" 
> object file, or a made-up one. And the "making-up" code can work 
> independently of there being a breakpad file.


Ok. Check out my changes that parse region info:
https://reviews.llvm.org/D55522

It parses the memory region info from the linux maps info if it is available. 
In breakpad generated minidumps, this will give us enough info to correctly 
create sections for all  object files in case #3!


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

https://reviews.llvm.org/D55434



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

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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

BTW: check my changes in: https://reviews.llvm.org/D55522
It will be interesting to you since it parses the linux maps info if it is 
available in breakpad generated minidump files. This will give us enough info 
to create correct sections for object files when we have no ELF file or no 
symbol file (ELF/DWARF or breakpad).


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