[Lldb-commits] [PATCH] D122988: [BOLT][DWARF] Add version 5 split dwarf support

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: bolt/include/bolt/Core/DebugData.h:375-377
+if (Optional DWOId = Unit.getDWOId())
+  return *DWOId;
+return Unit.getOffset();

ayermolo wrote:
> ayermolo wrote:
> > dblaikie wrote:
> > > That seems like a somewhat problematic API - returning two very different 
> > > kinds of data (the DWO_ID or the unit offset) seems quite easy to misuse 
> > > this?
> > The idea I had behind this APIS is for it to return unique ID representing 
> > the CU. As it applies to .debug_addr. For monolithic case is its offset 
> > with .debug_info for Split Dwarf case is its DWO ID. At least in my head 
> > viewed in that context the return data is the same. It's just something 
> > that uniquely identifies this CU, and logic is encapsulated in it.
> > 
> @dblaikie What do you think?
Could you use the offset consistently? The debug_addr section is always in the 
executable - the offset of the skeleton CU would be unique?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122988

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

[Lldb-commits] [PATCH] D122974: prevent ConstString from calling djbHash() more than once

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D122974#3481269 , @llunak wrote:

> In D122974#3480686 , @dblaikie 
> wrote:
>
>> In D122974#3424654 , @JDevlieghere 
>> wrote:
>>
>>> 
>
> ...
>
>>>   struct HashedStringRef {
>>> const llvm::StringRef str;
>>> const unsigned full_hash;
>>> const uint8_t hash; 
>>> HashedStringRef(llvm::StringRef s) : str(s), full_hash(djbHash(str)), 
>>> hash(hash(str)) {}
>>>   }
>
> ...
>
>> The external code shouldn't be able to create their own (ctor 
>> private/protected, etc) - the only way to get one is to call `StringMap` to 
>> produce one.
>
> But what if I don't want StringMap to compute the hash at all? There's still 
> that 10-15% of CPU time spent in djbHash() when debug info uses exactly[*] 
> that (and LLDB's index cache could be modified to store it). Which means it 
> can be useful for StringMap to allow accepting precomputed hash, and then 
> what purpose will that HashedStringRef have?

If that happens, yeah, there's probably no point protecting this API - though 
I'd be more cautious of that change/any change that supports serializing the 
hash as this could end up causing StringMap to have to have stability 
guarantees that might be unfavorable for other uses (see ABSL maps that 
intentionally randomize/reseed each instance to ensure users aren't depending 
on undocumented guarantees - this gives them the flexibility to update the hash 
algorithm with something better in the future without breaking users who might 
be serializing the hashes/relying on iteration order/etc)

If we want a structure that can use a stable hash - it might be at that point 
that we move the hash support entirely out of StringMap and make it pluggable, 
with the default implementation doing djbHash as before, and even the new 
"stable" one doing that too, but doing it explicitly/documenting what 
guarantees it requires (stability within a major version? Across major 
versions?)

& then I guess what the API looks like is still an open question - perhaps the 
default trait (the one without any stability guarantees) could have a private 
implementation and expose that to StringMap via friendship. The stable 
implementation can have a public implementation for hashing & then an API like 
the one proposed where they can be passed into StringMap (yeah, without any of 
the handle safety previously proposed) - and assert when it's passed in that it 
matches what the trait provides (under EXPENSIVE_CHECKS, probably).


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

https://reviews.llvm.org/D122974

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

[Lldb-commits] [PATCH] D122974: prevent ConstString from calling djbHash() more than once

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

> It doesn't make sense to require a stable hash algorithm for an internal 
> cache file.

It's at least a stronger stability requirement than may be provided before - 
like: stable across process boundaries, at least, by the sounds of it? yeah?
& then still raises the question for me what about minor version updates, is it 
expected to be stable across those?

It'd be a bit subtle to have to know when to go and update the lldb cache 
version number because this hash function has changed, for instance. It might 
be more suitable to have lldb explicitly request a known hash function rather 
than the generic one (even if they're identical at the moment) so updates to 
LLVM's core libraries don't subtly break the hashing/cache system here. (I 
would guess there's no cross-version hash testing? So it seems like such a 
change could produce fairly subtle breakage that would slip under the radar 
fairly easily?)


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

https://reviews.llvm.org/D122974

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

[Lldb-commits] [PATCH] D122974: prevent ConstString from calling djbHash() more than once

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D122974#3536342 , @llunak wrote:

> In D122974#3535669 , @dblaikie 
> wrote:
>
>>> It doesn't make sense to require a stable hash algorithm for an internal 
>>> cache file.
>>
>> It's at least a stronger stability requirement than may be provided before - 
>> like: stable across process boundaries, at least, by the sounds of it? yeah?
>
> It's meant to be the same for the same library binary.
>
>> & then still raises the question for me what about minor version updates, is 
>> it expected to be stable across those?
>
> Is anything expected to be stable across those? If so, that doesn't seem to 
> be the reality of it (a random quick check finds two ABI changes just between 
> 14.0.4 and 14.0.5, e6de9ed37308e46560243229dd78e84542f37ead 
>  and 
> 53433dd0b5034681e1866e74651e8527a29e9364 
> ).

My udnerstanding is that generally ABI stability is intended to be guaranteed 
across minor releases (& those patches don't look like ABI breaks to me - the 
first one changes the mangling of intended-to-be-local symbols so they don't 
collide, so it should cause valid programs to link when they previously failed 
to link. The second seems to add a new target that wasn't present - so nothing 
to break against?) but that's probably orthogonal to the stability of the 
map/cache/expectations of folks releasing and using lldb.

>> It'd be a bit subtle to have to know when to go and update the lldb cache 
>> version number because this hash function has changed, for instance. It 
>> might be more suitable to have lldb explicitly request a known hash function 
>> rather than the generic one (even if they're identical at the moment) so 
>> updates to LLVM's core libraries don't subtly break the hashing/cache system 
>> here. (I would guess there's no cross-version hash testing? So it seems like 
>> such a change could produce fairly subtle breakage that would slip under the 
>> radar fairly easily?)
>
> D124704  adds a unittest that compares 
> StringMap::hash() to a known hardcoded value, so whenever the hash 
> implementation changes, it won't be possible to unittest LLDB with that 
> change, and that will be the time to change the lldb cache version.

Ah, good stuff - doesn't guarantee that any hash change necessarily breaks the 
test, but certainly helps/seems like a good idea, thanks!

> If the theory is that this should keep working even with the library changing 
> without LLDB rebuild, then as I wrote above that theory needs double-checking 
> first. And additionally a good question to ask would be if it's really a good 
> idea to do incompatible implementation changes to a class that has half of 
> its functionality inline.

Sorry, I wasn't meaning to discuss dynamic library versioning 
issues/mismatches, just static/fully matched versions.

> Finally, there have been already attempts to change the hash function to use 
> the better non-zero seed (D97396 ), and they 
> were reverted because something depended on the hash function not changing, 
> so I don't see it that likely for the hash function to change just like that 
> in a minor update.

That something seems to have been another part of lldb - and that's the sort of 
change I'd like to enable/not make harder by avoiding more dependence on this 
ordering/hashing.

> But if after all this that's still the theory, I guess StringMap could get an 
> additional template parameter specifying the hash function, if it's actually 
> worth the effort.

Yeah, a little traits class or the like is roughly what I'd picture.


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

https://reviews.llvm.org/D122974

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

[Lldb-commits] [PATCH] D122974: prevent ConstString from calling djbHash() more than once

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

>>> If the theory is that this should keep working even with the library 
>>> changing without LLDB rebuild, then as I wrote above that theory needs 
>>> double-checking first. And additionally a good question to ask would be if 
>>> it's really a good idea to do incompatible implementation changes to a 
>>> class that has half of its functionality inline.
>>
>> Sorry, I wasn't meaning to discuss dynamic library versioning 
>> issues/mismatches, just static/fully matched versions.
>
> Then I still don't know what the problem is supposed to be. If the StringMap 
> hash implementation ever changes, the necessary LLDB rebuild will detect 
> this, the relevant LLDB parts will get adjusted and problem solved.

What I mean is if the cache is used across statically linked versions - eg: 
cache is created, someone installs an update to lldb, then the cache is read 
back and misinterprets the hashes in the cache if the hash algorithm had 
changed between versions.

>> Finally, there have been already attempts to change the hash function to use 
>> the better non-zero seed (D97396 ), and 
>> they were reverted because something depended on the hash function not 
>> changing, so I don't see it that likely for the hash function to change just 
>> like that in a minor update.
>
> That something seems to have been another part of lldb - and that's the sort 
> of change I'd like to enable/not make harder by avoiding more dependence on 
> this ordering/hashing.
>
>> But if after all this that's still the theory, I guess StringMap could get 
>> an additional template parameter specifying the hash function, if it's 
>> actually worth the effort.
>
> Yeah, a little traits class or the like is roughly what I'd picture.

^


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

https://reviews.llvm.org/D122974

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

[Lldb-commits] [PATCH] D122974: prevent ConstString from calling djbHash() more than once

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

>>> Finally, there have been already attempts to change the hash function to 
>>> use the better non-zero seed (D97396 ), 
>>> and they were reverted because something depended on the hash function not 
>>> changing, so I don't see it that likely for the hash function to change 
>>> just like that in a minor update.
>>
>> That something seems to have been another part of lldb - and that's the sort 
>> of change I'd like to enable/not make harder by avoiding more dependence on 
>> this ordering/hashing.
>>
>>> But if after all this that's still the theory, I guess StringMap could get 
>>> an additional template parameter specifying the hash function, if it's 
>>> actually worth the effort.
>>
>> Yeah, a little traits class or the like is roughly what I'd picture.
>
> ^

^ I think it's still worthwhile/necessary to separate LLDB's use case/hashing 
algorithm choice from LLVM's so LLVM's code can be changed to be more change 
resilient in a way that LLDB's cannot (eg: random seeds will never be usable by 
LLDB but may be for LLVM).

In D122974#3567444 , @llunak wrote:

> In D122974#3567278 , @dblaikie 
> wrote:
>
>>> Then I still don't know what the problem is supposed to be. If the 
>>> StringMap hash implementation ever changes, the necessary LLDB rebuild will 
>>> detect this, the relevant LLDB parts will get adjusted and problem solved.
>>
>> What I mean is if the cache is used across statically linked versions - eg: 
>> cache is created, someone installs an update to lldb, then the cache is read 
>> back and misinterprets the hashes in the cache if the hash algorithm had 
>> changed between versions.
>
> That is not going to happen, that updated LLDB will be using a different 
> cache version. That's what the part you're replying to means. You cannot have 
> two LLDB builds using the same cache version but different hash 
> implementations, as long as you do not ignore LLDB unittests failing.

Fair enough - probably good to have some commentary in the test cases that 
makes it really clear that if the test needs to be updated then the version 
needs to be updated. (is that patch already posted? Could you link to that 
comment from this review?)


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

https://reviews.llvm.org/D122974

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

[Lldb-commits] [PATCH] D129166: [lldb] Make sure we use the libc++ from the build dir

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

> The overall logic and the include and library paths make sense to me. The 
> rpath thingy will not work on windows as it (afaik) has no equivalent feature 
> (it has the equivalent of (DY)LD_LIBRARY_PATH though).

Any idea what the libc++ tests do on Windows then? (on linux they seem to use 
rpath to ensure the tests load the just-built libc++)


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

https://reviews.llvm.org/D129166

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

[Lldb-commits] [PATCH] D90598: [lldb/DWARF] Fix implementation of DW_OP_convert

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D90598#2368020 , @labath wrote:

> Judging by http://lists.llvm.org/pipermail/llvm-dev/2020-October/145990.html, 
> dsymutil has a similar bug...

That's my understanding. It means I'm not sure (though I'm not the expert here 
- certainly leave final decisions up to @aprantl and @JDevlieghere ) how this 
will be fixed, since lldb will presumably still need to be able to 
parse/understand old dsyms using the incorrect absolute offsets. (we had a 
similar problem once with DW_OP_bit_piece, where it was implemented incorrectly 
in both LLVM and lldb and I'm not sure how the backwards compatibility story 
was addressed there).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90598

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

[Lldb-commits] [PATCH] D90840: [lldb/DWARF] Fix sizes of DW_OP_const[1248][us] and DW_OP_litN results

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: lldb/source/Expression/DWARFExpression.cpp:944
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
+  auto to_generic = [addr_size = opcodes.GetAddressByteSize()](auto v) {
+bool is_signed = std::is_signed::value;

Local/non-escaping lambdas may be simpler written with default ref capture - is 
GetAddressByteSize() expensive or would it be fine to call it every time this 
lambda is called? (& capturing opcodes by ref)



Comment at: lldb/source/Expression/DWARFExpression.cpp:945
+  auto to_generic = [addr_size = opcodes.GetAddressByteSize()](auto v) {
+bool is_signed = std::is_signed::value;
+return Scalar(

labath wrote:
> This is probably not conforming either -- dwarf says the generic type is of 
> "unspecified signedness", but I think it means a single global value -- not 
> choosing signedness on a per-operand basis.
> 
> I've kept that for now, since that was the original behavior, though I didn't 
> notice any issues with this bit removed.
Fair, would be good to mark it as at least suspect/uncertain with a comment or 
such. (or if there's some inverse that would assert if this ever did become 
relevant, that assertion would be great - but I guess there probably isn't 
anything we can assert to check that this doesn't matter in practice)

Re: non-conforming: I think the presence of constu and consts encodings that 
build "generic type"d values probably means the generic type itself may have no 
particular notion of signedness, but the values that populate it do and I guess 
intend the value to be interpreted contextually or to carry the sign 
information? I think it's probably good to carry the sign information from the 
operands, at least. Maybe with some truncation/sign extension. It's probably 
pretty close to right... 



Comment at: lldb/source/Expression/DWARFExpression.cpp:1270
 case DW_OP_const1u:
-  stack.push_back(Scalar((uint8_t)opcodes.GetU8(&offset)));
+  stack.emplace_back(to_generic(opcodes.GetU8(&offset)));
   break;

Why the change to emplace_back? Generally I'd advocate for push_back where 
possible, since it can't invoke explicit conversions which can require more 
scrutiny (eg: a vector of unique_ptrs can have a unique_ptr rvalue push_back'd, 
but can't have a raw pointer push_back'd (a raw pointer would have to be 
emplace_back'd) - so when I see push_back I can assume it's a "safe" operation)



Comment at: lldb/source/Expression/DWARFExpression.cpp:1293-1300
+// These should also use to_generic, but we can't do that due to a
+// producer-side bug in llvm. See llvm.org/pr48087.
 case DW_OP_constu:
-  stack.push_back(Scalar(opcodes.GetULEB128(&offset)));
+  stack.emplace_back(opcodes.GetULEB128(&offset));
   break;
 case DW_OP_consts:
+  stack.emplace_back(opcodes.GetSLEB128(&offset));

Seems fair


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90840

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

[Lldb-commits] [PATCH] D90840: [lldb/DWARF] Fix sizes of DW_OP_const[1248][us] and DW_OP_litN results

[David Blaikie via Phabricator via lldb-commits]

dblaikie accepted this revision.
dblaikie added a comment.

Sounds good


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90840

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

[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

This causes several regressions in the gdb test suite we're running internally 
- the failures are:

  FAIL: gdb.base/break.exp: breakpoint at start of multi line while conditional
  FAIL: gdb.base/break.exp: breakpoint info
  FAIL: gdb.base/foll-exec.exp: step through execlp call
  FAIL: gdb.base/foll-exec.exp: step after execlp call
  FAIL: gdb.base/foll-exec.exp: print execd-program/global_i (after execlp)
  FAIL: gdb.base/foll-exec.exp: print execd-program/local_j (after execlp)
  FAIL: gdb.base/foll-exec.exp: print follow-exec/local_k (after execlp)
  FAIL: gdb.base/hbreak2.exp: hardware breakpoint at start of multi line while 
conditional
  FAIL: gdb.base/hbreak2.exp: hardware breakpoint info

I looked into the first one in break.exp and reduced it a bit, given code like 
this:

  int main() { }
  int f1(int a, int b) {
return a && b;
  }

debugging this binary with gdb and running `b f1` results in breaking on an 
instruction with no line information after this change, eg:
old behavior:

  (gdb) b f1
  Breakpoint 1 at 0x40112c: file test.c, line 2.

new behavior:

  (gdb) b f1
  Breakpoint 1 at 0x40112a

The assembly difference:

  -   xorl%eax, %eax
  -# kill: def $al killed $al killed 
$eax
  movl%edi, -4(%rbp)
  movl%esi, -8(%rbp)
   .Ltmp2:
  -   .loc1 3 10 prologue_end # break.c:3:10
  +   .loc1 0 0 prologue_end  # break.c:0:0
  +   xorl%eax, %eax
  +# kill: def $al killed $al killed 
$eax
  +   .loc1 3 10  # break.c:3:10
  cmpl$0, -4(%rbp)
  movb%al, -9(%rbp)   # 1-byte Spill

So it looks like the && caused a location-less zero constant to be synthesized 
and that previously floated up into the prologue - but now it's pulled down 
past the prologue begin, so after the prologue this location-less zero is the 
first location.

I've reverted this patch (& dependent/follow-on patches) in 
615f63e149f31d6a97b5233b4fe634db92e19aa9 
 - happy 
to help with further reproductions/validations (if/when you've got a proposed 
fix, if you can't run the gdb test suite or otherwise reproduce the failures, I 
can run our internal infrastructure to validate again)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91734

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

[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D91734#2426897 , @rnk wrote:

> I see. We should give that constant materialization a location.

Yeah, likely - if this patch makes constant materialization local to the IR 
instruction - if there's a reasonable location the IR instruction should have, 
that seems fair to me.

(a more general fix (that would cover cases where the instruction really has no 
location) might be to propagate locations from the first instruction in a basic 
block with a location back up to the start of the basic block - I forget if 
we've considered/tried that before)

> It looks like it is coming from a phi node. The IR looks like this:
>
> %6 = icmp ne i32 %5, 0, !dbg !11
> br i1 %6, label %7, label %10, !dbg !12
>   
>   7:; preds = %2
> %8 = load i32, i32* %4, align 4, !dbg !13
> %9 = icmp ne i32 %8, 0, !dbg !13
> br label %10
>   
>   10:   ; preds = %7, %2
> %11 = phi i1 [ false, %2 ], [ %9, %7 ], !dbg !14    Probably where 
> the zero comes from
> %12 = zext i1 %11 to i32, !dbg !11
> ret i32 %12, !dbg !15
>
> The PHI node has location !14, which is a line 0 location. Is there a reason 
> we give this PHI a line 0 location, when it's built by the frontend for the 
> conditional operator? IMO we should use the location of the `br` instruction, 
> which will be the location of the conditional operator (`&&` at the source 
> level).
>
> Paul has already shown that flushing the local value map improves debug info 
> quality in general. If we can't fix all the gdb test suite failures, IMO we 
> should consider XFAILing them and moving on.

For sure there's tipping points where one bug is worth another - but generally 
it's best to avoid that if possible. (& a sliding scale of "can be fixed" - 
matter of code, complexity, etc - yeah, if the alternative is rewriting big 
parts of LLVM to avoid regressing one thing to progress here - I'm with you, 
there's some place where we'd tradeoff some bugs for some other bugs)

In D91734#2429144 , @probinson wrote:

> Just for grins, change the `&&` to `||` and see what happens...
> The xorl becomes a movb $1 (no surprise there).  But, that instruction no 
> longer has line-0, instead it becomes part of the prologue.
> This tells me that the xorl had an explicit line 0, while the 'movb $1' has 
> no location (a subtle but important difference).
>
> There are also curious differences between the CGExprScalar.cpp methods 
> VisitBinLAnd() and VisitBinLOr(); the former is more attentive to line 
> attributions than the latter, which seems likely to be an oversight dating 
> back a decade or so.
>
> Onward into the breach.

I did some work years back to try to streamline the location of instructions 
when code generating expressions in clang. Mostly centered around the creation 
and use of `ApplyDebugLocation` scoped type. Which you can see used in various 
places, but for instance, at the start of `CodeGenFunction::EmitLValue(const 
Expr *E)` - the idea being that all instructions related to code generating 
that expression would have a location the same as the preferred location (the 
same place clang points to in error messages) of the expression - and any 
subexpression would get a similar treatment, overriding the outer expression's 
location in turn.

Let's see what's up with VisitBinLAnd and VisitBinLOr... Ah, you're referring 
to the use of ApplyDebugLocation around the two EmitBlock calls in VisitBinLAnd 
that are missing in VisitBinLOr? Yeah, looks like an omission on my part back 
when - I vaguely remember that whole "unconditional branches shouldn't have 
locations" thing, but hopefully there's enough info in the commit messages to 
help explain why it's useful as I don't think I have that much context anymore.

Right you are about line 0 V no location - the phi after the && has line 0, but 
the phi after the || has no line.

Here's where the line 0 for the Phi is coming from: 
https://github.com/llvm-mirror/clang/blob/master/lib/CodeGen/CGExprScalar.cpp#L4039
 - I was going to blame myself, but seems to have come from here: 
https://reviews.llvm.org/D47720

This patch might be undoing, awkwardly, the point of my original patch of 
omitting locations on unconditional jumps... Hmm, nope, that's just about the 
jumps.

Looks like this Phi creation ( 
https://github.com/llvm-mirror/clang/blob/master/lib/CodeGen/CGExprScalar.cpp#L4012
 ) misses the opportunity to get the location from the expression, because it's 
not created by the IRBuilder - perhaps it should be, rather than being given an 
artificial location - we know which expression it's coming from, just as much 
as we know the instruction for the loads, etc. So maybe that code should be 
`ApplyDebugLocation::CreateArtificial` and scope, and instead set the location 
to the current debug loca

[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

Sometihng like this seems plausible to me:

  $ git diff
  diff --git clang/lib/CodeGen/CGExprScalar.cpp 
clang/lib/CodeGen/CGExprScalar.cpp
  index c906af8a4afa..c85ce46508a6 100644
  --- clang/lib/CodeGen/CGExprScalar.cpp
  +++ clang/lib/CodeGen/CGExprScalar.cpp
  @@ -4187,6 +4187,7 @@ Value *ScalarExprEmitter::VisitBinLAnd(const 
BinaryOperator *E) {
 // setting up the PHI node in the Cont Block for this.
 llvm::PHINode *PN = 
llvm::PHINode::Create(llvm::Type::getInt1Ty(VMContext), 2,
   "", ContBlock);
  +  PN->setDebugLoc(Builder.getCurrentDebugLocation());
 for (llvm::pred_iterator PI = pred_begin(ContBlock), PE = 
pred_end(ContBlock);
  PI != PE; ++PI)
   PN->addIncoming(llvm::ConstantInt::getFalse(VMContext), *PI);
  @@ -4209,12 +4210,6 @@ Value *ScalarExprEmitter::VisitBinLAnd(const 
BinaryOperator *E) {
 // Insert an entry into the phi node for the edge with the value of 
RHSCond.
 PN->addIncoming(RHSCond, RHSBlock);
  
  -  // Artificial location to preserve the scope information
  -  {
  -auto NL = ApplyDebugLocation::CreateArtificial(CGF);
  -PN->setDebugLoc(Builder.getCurrentDebugLocation());
  -  }
  -
 // ZExt result to int.
 return Builder.CreateZExtOrBitCast(PN, ResTy, "land.ext");
   }
  diff --git clang/test/CodeGen/debug-info-inline-for.c 
clang/test/CodeGen/debug-info-inline-for.c
  index 55066b28a1ff..5d1b1e7fc3bd 100644
  --- clang/test/CodeGen/debug-info-inline-for.c
  +++ clang/test/CodeGen/debug-info-inline-for.c
  @@ -10,4 +10,4 @@ int func(int n) {
   // CHECK: land.end:
   // CHECK-NEXT: {{.*}} = phi i1 {{.*}} !dbg ![[DbgLoc:[0-9]+]]
  
  -// CHECK: ![[DbgLoc]] = !DILocation(line: 0
  +// CHECK: ![[DbgLoc]] = !DILocation(line: 6, column: 19

Not sure if it addresses all of the regressions though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91734

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

[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D91734#2431247 , @probinson wrote:

>> Sometihng like this seems plausible to me:
>
> Yes, I was playing with essentially that exact patch last night.  It has no 
> effect on the final assembly on its own, but combined with my patch it does.

It might have effects on assembly in other test cases, though. Could be worth 
running it through a self-host or something to see what other changes it causes 
and whether they're desirable.

>> (a more general fix (that would cover cases where the instruction really has 
>> no location) might be to propagate locations from the first instruction in a 
>> basic block with a location back up to the start of the basic block - I 
>> forget if we've considered/tried that before)
>
> We have, but that without flushing the map on every instruction, so it caught 
> materialization instructions that didn't actually belong to that IR 
> instruction.  The tactic would likely be more reasonable in conjunction with 
> my patch.

(oh, when I was saying that I didn't really think - the materialization in this 
case wasn't necessarily on a BB boundary - so I guess my suggestion amounts to 
possibly never using line 0 (unless it's the only location in a whole BB), 
instead back or forward propagating surrounding locations over any line 0 - and 
that doesn't sound right when I put it that way)

But yeah, maybe some amount of it could be done around the flushing thing.

(FWIW, about this patch in general, I do worry a bit about this being a 
debug-info motivated optimization decision (even if that decision is applied 
uniformly/not just when debug info is enabled) - you mention this might have 
positive performance features due to smaller register live ranges, but also 
possibly negative ones rematerializing the same constant (" however, only about 
5% of those values
were reused in an experimental self-build of clang.") - do you have performance 
measurements/benchmarks related to this change? I guess it didn't show up in 
the perf bot profiles upstream at least)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91734

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

[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D91734#2432188 , @probinson wrote:

> See D92606  for a front-end patch to improve 
> locations in the IR.
> That, plus reapplying this patch, should help out GDB.  I haven't had a 
> chance to run the suite myself with both patches applied and I'm basically 
> off tomorrow, so if @dblaikie doesn't have a chance, I'll do that on Monday.

Had a go - certainly creates a better debugging experience, but still fails the 
gdb test in question.

The code in the original test looks something like:

  int multi_line_while_conditional (int a, int b, int c)
  {
while (a /* set breakpoint 4 here */
&& b 
&& c) 
  { 
a--, b--, c--;
  }
return 0;
  }

And with these changes together, breaking on the function breaks on line 5 
(presumably because this is creating the constant used by the second '&&' which 
is on line 5) instead of line 3. Not the worst thing, but I imagine given 
Sony's interest in less "jumpy" line tables, this might not be super desirable.

Yeah, the gdb experience is less than ideal:

  13multi_line_while_conditional(1, 1, 1);
  (gdb) s
  multi_line_while_conditional (a=1, b=1, c=1) at test.cpp:5
  5 && c) 
  (gdb) n
  3 while (a /* set breakpoint 4 here */
  (gdb) 
  4 && b 
  (gdb) 
  5 && c) 
  (gdb) 
  4 && b 
  (gdb) 
  5 && c) 
  (gdb) 
  3 while (a /* set breakpoint 4 here */
  (gdb) 
  7 a--, b--, c--;

Compared with (without any of these patches):

  13multi_line_while_conditional(1, 1, 1);
  (gdb) s
  multi_line_while_conditional (a=1, b=1, c=1) at test.cpp:3
  3 while (a /* set breakpoint 4 here */
  (gdb) n
  4 && b 
  (gdb) 
  5 && c) 
  (gdb) 
  3 while (a /* set breakpoint 4 here */
  (gdb) 
  7 a--, b--, c--;

And because I was curious about /exactly/ which tokens the debugger was 
stepping to, I split out the tokens onto their own lines:
With patch:

  18multi_line_while_conditional(1, 1, 1);
  (gdb) s
  multi_line_while_conditional (a=1, b=1, c=1) at test.cpp:8
  8 && 
  (gdb) n
  5   a 
  (gdb) 
  6 && 
  (gdb) 
  8 && 
  (gdb) 
  7 b 
  (gdb) 
  8 && 
  (gdb) 
  9 c
  (gdb) 
  3 while 
  (gdb) 
  12a--, b--, c--;

Without patch/with trunk:

  18multi_line_while_conditional(1, 1, 1);
  (gdb) s
  multi_line_while_conditional (a=1, b=1, c=1) at test.cpp:5
  5   a 
  (gdb) n
  6 && 
  (gdb) 
  7 b 
  (gdb) 
  8 && 
  (gdb) 
  9 c
  (gdb) 
  3 while 
  (gdb) 
  12a--, b--, c--;

Maybe it's OK? But at least it's 'interesting' enough that might deserve some 
extra scrutiny.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91734

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

[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

Also, FWIW, the other gdb test suite failures we saw were:

  FAIL: gdb.base/foll-exec.exp: step through execlp call
  FAIL: gdb.base/foll-exec.exp: step after execlp call
  FAIL: gdb.base/foll-exec.exp: print execd-program/global_i (after execlp)
  FAIL: gdb.base/foll-exec.exp: print execd-program/local_j (after execlp)
  FAIL: gdb.base/foll-exec.exp: print follow-exec/local_k (after execlp)
  FAIL: gdb.base/hbreak2.exp: hardware breakpoint at start of multi line while 
conditional 
  FAIL: gdb.base/hbreak2.exp: hardware breakpoint info

Those last two are different versions of this original issue seen in 
break.exp/c with multi-line conditionals.

The foll-exec.exp ones I'm less sure of - Looks like maybe it's a similar line 
table attribution sort of thing, but it leads the test case not to step across 
the intended process boundary/exec call and things go weird from there.

Yeah, it boils down to something like this:

  void f1(const char*, const char*) { }
  int main() {
char prog[1];
  
f1(prog,
   prog);
  }

Without the patch, you step to the 'f1' line, and then step into or over the 
function call (step or next).
But with these patches applied - you step to the 'f1(' line, then the 'prog);' 
line, then back to the 'f1(' line, then into/over the function call.

Again, could be acceptable - if those arguments had specific non-trivial code 
in them (like other function calls) you'd certainly get that step 
forward/backward behavior - but it's notewhorthy that this is change would make 
more cases like that & it'd be good to understand why/if that's a good thing 
overall.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91734

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

[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D91734#2443231 , @probinson wrote:

> In D91734#2432988 , @dblaikie wrote:
>
>> Yeah, it boils down to something like this:
>>
>>   void f1(const char*, const char*) { }
>>   int main() {
>> char prog[1];
>>   
>> f1(prog,
>>prog);
>>   }
>>
>> Without the patch, you step to the 'f1' line, and then step into or over the 
>> function call (step or next).
>> But with these patches applied - you step to the 'f1(' line, then the 
>> 'prog);' line, then back to the 'f1(' line, then into/over the function call.
>>
>> Again, could be acceptable - if those arguments had specific non-trivial 
>> code in them (like other function calls) you'd certainly get that step 
>> forward/backward behavior - but it's notewhorthy that this is change would 
>> make more cases like that & it'd be good to understand why/if that's a good 
>> thing overall.
>
> If you dump the IR you should see two GEP instructions, one for each actual 
> parameter expression.  Prior to the patch, FastISel found the second one 
> computed the same value as the first one, and reused the value (in this case, 
> the result of `lea`).  This is one of the 5% of cases where a value is being 
> reused across instructions, with the HEAD compiler. After the patch, we flush 
> the local map after each GEP, so the second one gets its own separate `lea` 
> (with its own source location).  So, you're stepping to the first actual, 
> then to the second, then to the call.  Hope that answers the "why" question.
>
> Whether it's a "good" thing... that path is fraught with peril.  Looking at 
> it somewhat abstractly (as I am sometimes wont to do), Clang is making 
> choices to associate the source location of each parameter with the IR 
> instructions to compute that parameter, and by flushing the map on every IR 
> instruction, LLVM is more faithfully representing that in the final object.  
> This is -O0 after all, where a more faithful representation is more likely to 
> be more debuggable.  gcc is making entirely different choices, lumping all 
> parameter-prep instructions in with the function call itself.  I'm hard 
> pressed to say one is better than the other; they are different.  Coke and 
> Pepsi; people may have preferences, but that's not the same as being higher 
> on a good-ness scale.

Fair enough - if you're satisfied these are reasonable cases of 
different-but-reasonable DWARF locations for the instructions, I'm OK with it.

If possible, would be good if you could run the gdb test suite and document the 
"regressions" (and possibly document test case changes you'd make to keep those 
tests passing) with this change - if you're liable to do that sooner or later 
anyway for your own test infrastructure. Otherwise I'll just handle it on our 
end.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91734

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

[Lldb-commits] [PATCH] D94063: lldb: Add support for DW_AT_ranges on DW_TAG_subprograms

[David Blaikie via Phabricator via lldb-commits]

dblaikie created this revision.
dblaikie added a reviewer: labath.
dblaikie requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added subscribers: lldb-commits, sstefan1.
Herald added a project: LLDB.

gcc already produces debug info with this form
-freorder-block-and-partition
clang produces this sort of thing with -fbasic-block-sections and with a
coming-soon tweak to use ranges in DWARFv5 where they can allow greater
reuse of debug_addr than the low/high_pc forms.

This fixes the case of breaking on a function name, but leaves broken
printing a variable - a follow-up commit will add that and improve the
test case to match.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D94063

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
  lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
  lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test

Index: lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
@@ -0,0 +1,29 @@
+# UNSUPPORTED: system-windows
+# RUN: %clang_host -g -O0 %S/Inputs/subprogram_ranges.s -o %t.out
+# RUN: not %lldb -b -s %s %t.out 2>&1 | FileCheck %s
+
+# Test breaking on symbols and printing variables when a DW_TAG_subprogram uses
+# DW_AT_ranges instead of DW_AT_low_pc/DW_AT_high_pc.  While the assembly here
+# is a bit unrealistic - it's a single-entry range using DWARFv4 which isn't
+# useful for anything (a single-entry range with DWARFv5 can reduce address
+# relocations, and multi-entry ranges can be used for function sections), but
+# it's the simplest thing to test. If anyone's updating this test at some
+# point, feel free to replace it with another equivalent test if it's
+# especially useful, but don't dismiss it as pointless just because it's a bit
+# weird.
+
+b main
+# CHECK: (lldb) b main
+# CHECK-NEXT: Breakpoint 1: where = subprogram_ranges.test.tmp.out`main + 6 at main.c:2:7
+
+run
+# Stopping inside of the stop hook range
+# CHECK: (lldb) run
+
+print var
+# Check that local variable names can be looked up
+# FIXME: This should be: (int) $0 = {{.*}}
+# CHECK: (lldb) print var
+# CHECK-NEXT: error: :1:1: use of undeclared identifier 'var'
+
+q
Index: lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
@@ -0,0 +1,159 @@
+	.text
+	.file	"main.c"
+	.globl	main# -- Begin function main
+	.p2align	4, 0x90
+	.type	main,@function
+main:   # @main
+.Lfunc_begin0:
+	.file	1 "/usr/local/google/home/blaikie/dev/scratch" "main.c"
+	.loc	1 1 0   # main.c:1:0
+	.cfi_startproc
+# %bb.0:# %entry
+	pushq	%rbp
+	.cfi_def_cfa_offset 16
+	.cfi_offset %rbp, -16
+	movq	%rsp, %rbp
+	.cfi_def_cfa_register %rbp
+	xorl	%eax, %eax
+.Ltmp0:
+	.loc	1 2 7 prologue_end  # main.c:2:7
+	movl	$3, -4(%rbp)
+	.loc	1 3 1   # main.c:3:1
+	popq	%rbp
+	.cfi_def_cfa %rsp, 8
+	retq
+.Ltmp1:
+.Lfunc_end0:
+	.size	main, .Lfunc_end0-main
+	.cfi_endproc
+# -- End function
+	.section	.debug_abbrev,"",@progbits
+	.byte	1   # Abbreviation Code
+	.byte	17  # DW_TAG_compile_unit
+	.byte	1   # DW_CHILDREN_yes
+	.byte	37  # DW_AT_producer
+	.byte	14  # DW_FORM_strp
+	.byte	19  # DW_AT_language
+	.byte	5   # DW_FORM_data2
+	.byte	3   # DW_AT_name
+	.byte	14  # DW_FORM_strp
+	.byte	16  # DW_AT_stmt_list
+	.byte	23  # DW_FORM_sec_offset
+	.byte	27  # DW_AT_comp_dir
+	.byte	14  # DW_FORM_strp
+	.byte	17  # DW_AT_low_pc
+	.byte	1   # DW_FORM_addr
+	.byte	18  # DW_AT_high_pc
+	.byte	6   # DW_FORM_data4
+	.byte	0   # EOM(1)
+	.byte	0   # EOM(2)
+	.byte	2   # Abbreviation Code
+	.byte	46  # DW_TAG_subprogram
+	.byte	1   # DW_CHILDREN_yes
+	.byte	85  # DW_AT_ranges
+	.byte	23  # DW_FORM_sec_offset
+	.byte	64  # DW_AT_frame_base
+	.byte	24  # DW_FORM_exprloc
+	.byte	3   # DW_AT_name

[Lldb-commits] [PATCH] D94064: lldb: Add support for printing variables with DW_AT_ranges on DW_TAG_subprograms

[David Blaikie via Phabricator via lldb-commits]

dblaikie created this revision.
dblaikie added a reviewer: labath.
dblaikie requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Finishing out the support (to the best of my knowledge/based on current
testing running the whole check-lldb with a clang forcibly using
DW_AT_ranges on all DW_TAG_subprograms) for this feature.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D94064

Files:
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test


Index: lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
===
--- lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
+++ lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
@@ -1,6 +1,6 @@
 # UNSUPPORTED: system-windows
 # RUN: %clang_host -g -O0 %S/Inputs/subprogram_ranges.s -o %t.out
-# RUN: not %lldb -b -s %s %t.out 2>&1 | FileCheck %s
+# RUN: %lldb -b -s %s %t.out 2>&1 | FileCheck %s
 
 # Test breaking on symbols and printing variables when a DW_TAG_subprogram uses
 # DW_AT_ranges instead of DW_AT_low_pc/DW_AT_high_pc.  While the assembly here
@@ -22,8 +22,7 @@
 
 print var
 # Check that local variable names can be looked up
-# FIXME: This should be: (int) $0 = {{.*}}
 # CHECK: (lldb) print var
-# CHECK-NEXT: error: :1:1: use of undeclared identifier 
'var'
+# CHECK-NEXT: (int) $0 = {{.*}}
 
 q
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -3040,8 +3040,13 @@
 if (sc.function) {
   DWARFDIE function_die = GetDIE(sc.function->GetID());
 
-  const dw_addr_t func_lo_pc = function_die.GetAttributeValueAsAddress(
+  dw_addr_t func_lo_pc = function_die.GetAttributeValueAsAddress(
   DW_AT_low_pc, LLDB_INVALID_ADDRESS);
+  DWARFFormValue form_value;
+  if (func_lo_pc == LLDB_INVALID_ADDRESS &&
+  function_die.GetDIE()->GetAttributeValue(function_die.GetCU(),
+   DW_AT_ranges, form_value))
+func_lo_pc = 0;
   if (func_lo_pc != LLDB_INVALID_ADDRESS) {
 const size_t num_variables = ParseVariables(
 sc, function_die.GetFirstChild(), func_lo_pc, true, true);


Index: lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
===
--- lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
+++ lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
@@ -1,6 +1,6 @@
 # UNSUPPORTED: system-windows
 # RUN: %clang_host -g -O0 %S/Inputs/subprogram_ranges.s -o %t.out
-# RUN: not %lldb -b -s %s %t.out 2>&1 | FileCheck %s
+# RUN: %lldb -b -s %s %t.out 2>&1 | FileCheck %s
 
 # Test breaking on symbols and printing variables when a DW_TAG_subprogram uses
 # DW_AT_ranges instead of DW_AT_low_pc/DW_AT_high_pc.  While the assembly here
@@ -22,8 +22,7 @@
 
 print var
 # Check that local variable names can be looked up
-# FIXME: This should be: (int) $0 = {{.*}}
 # CHECK: (lldb) print var
-# CHECK-NEXT: error: :1:1: use of undeclared identifier 'var'
+# CHECK-NEXT: (int) $0 = {{.*}}
 
 q
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -3040,8 +3040,13 @@
 if (sc.function) {
   DWARFDIE function_die = GetDIE(sc.function->GetID());
 
-  const dw_addr_t func_lo_pc = function_die.GetAttributeValueAsAddress(
+  dw_addr_t func_lo_pc = function_die.GetAttributeValueAsAddress(
   DW_AT_low_pc, LLDB_INVALID_ADDRESS);
+  DWARFFormValue form_value;
+  if (func_lo_pc == LLDB_INVALID_ADDRESS &&
+  function_die.GetDIE()->GetAttributeValue(function_die.GetCU(),
+   DW_AT_ranges, form_value))
+func_lo_pc = 0;
   if (func_lo_pc != LLDB_INVALID_ADDRESS) {
 const size_t num_variables = ParseVariables(
 sc, function_die.GetFirstChild(), func_lo_pc, true, true);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D94136: [lldb] Make ModuleList iterable (NFC)

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: lldb/include/lldb/Core/ModuleList.h:70
+class ModuleIterator
+: public std::iterator {
+public:

FWIW, std::iterator is deprecated since C++17 - probably best not to add new 
uses of it. (I think the idea is that the typedefs that std::iterator provides 
are no longer needed because of auto/decltype/etc, but I might be wrong there 
(would have to check the iterator concepts specifications))

If you need some utility help with implementing an iterator, llvm's 
iterator_facade_base might help by allowing a fairly minimal implementation to 
autogenerate various symmetric members, etc.


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

https://reviews.llvm.org/D94136

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

[Lldb-commits] [PATCH] D94136: [lldb] Make ModuleList iterable (NFC)

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: lldb/include/lldb/Core/ModuleList.h:71
+: public llvm::iterator_facade_base<
+  ModuleIterator, std::bidirectional_iterator_tag, lldb::ModuleSP> {
+public:

On the fence, but this could be a random access iterator, since it's only based 
on an integer index - would be fairly easy to implement, but I can appreciate 
not wanting to add more iterator surface area than you need for range-based-for 
loops.



Comment at: lldb/include/lldb/Core/ModuleList.h:76
+
+  const lldb::ModuleSP operator*() const;
+

Returning const value types is particularly weird/problematic in some ways - it 
prevents any moving from the return value. So should probably drop the "const" 
here.



Comment at: lldb/include/lldb/Core/ModuleList.h:93-96
+  friend bool operator!=(const ModuleIterator &lhs, const ModuleIterator &rhs) 
{
+return !(lhs == rhs);
+  }
+

Seems you don't need to implement `!=` - iterator_facade_base will provide that 
for you based on op==, I think?



Comment at: lldb/include/lldb/Core/ModuleList.h:98
+private:
+  const ModuleList &m_module_list;
+  size_t m_index;

This being a reference (rather than, say, a pointer) would prevent the iterator 
from being assignable, I think? Which isn't ideal for many iterator algorithms, 
etc.


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

https://reviews.llvm.org/D94136

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

[Lldb-commits] [PATCH] D94136: [lldb] Make ModuleList iterable (NFC)

[David Blaikie via Phabricator via lldb-commits]

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

Looks good to me with a few minor cleanups.




Comment at: lldb/include/lldb/Core/ModuleList.h:73-74
+public:
+  explicit ModuleIterator(const ModuleList *module_list, size_t idx);
+  ModuleIterator(const ModuleList *module_list);
+

Interesting choice to make the single-arg ctor implicit but the multi-arg 
explicit (usually people end up the other way around - because until C++ 17 or 
something, explicit was only relevant for single-arg constructors). In any case 
the ctors are only used once each & probably might as well have them both 
explicit.

Or maybe make only one ctor that takes the index explicitly and use it from 
begin/end - not sure there's much value added by having two ctors here, and 
honestly I'd have expected the no-arg version to start at zero, and the 
arg-taking version to start wherever you want, and I see they're the opposite, 
so that'd probably reinforce the idea that just having the one that takes an 
explicit index might be less confusing.



Comment at: lldb/include/lldb/Core/ModuleList.h:78-87
+  ModuleIterator &operator++() {
+++m_index;
+return *this;
+  }
+
+  ModuleIterator &operator--() {
+--m_index;

I think you can drop these two now - they'll be provided by the iterator facade.


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

https://reviews.llvm.org/D94136

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

[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

[David Blaikie via Phabricator via lldb-commits]

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

In D91734#2480474 , @probinson wrote:

> This version of the patch avoids the weirdness I was seeing with prolog 
> instructions in certain cases.
>
> The gdb test suite is largely happy with this; it avoids the new failures I 
> mentioned previously, and one test needed to have some "next" commands 
> updated.  Here's the gdb test suite patch (against 8.3; I can provide more 
> context if it's not clear how to apply to later versions).
>
>   diff --git a/gdb-8.3/gdb/testsuite/gdb.base/foll-exec.exp 
> b/gdb-8.3/gdb/testsuite/gdb.base/foll-exec.exp
>   index 3e0653a1..61ee752f 100644
>   --- a/gdb-8.3/gdb/testsuite/gdb.base/foll-exec.exp
>   +++ b/gdb-8.3/gdb/testsuite/gdb.base/foll-exec.exp
>   @@ -115,7 +115,10 @@ proc do_exec_tests {} {
>   # We should stop in execd-program, at its first statement.
>   #
>   set execd_line [gdb_get_line_number "after-exec" $srcfile2]
>   -   send_gdb "next\n"
>   +# Clang will emit source locations for the parameter evaluation.
>   +# Ideally we'd "next" until we saw 'execlp' again in the source 
> display,
>   +# then do one more.
>   +   send_gdb "next 3\n"
>   gdb_expect {
> -re ".*xecuting new program: 
> .*${testfile2}.*${srcfile2}:${execd_line}.*int  local_j = argc;.*$gdb_prompt 
> $"\
> {pass "step through execlp call"}
>   @@ -263,7 +266,7 @@ proc do_exec_tests {} {
>   # the newly-exec'd program, not after the remaining step-count
>   # reaches zero.
>   #
>   -   send_gdb "next 2\n"
>   +   send_gdb "next 3\n"
>   gdb_expect {
> -re ".*xecuting new program: 
> .*${testfile2}.*${srcfile2}:${execd_line}.*int  local_j = argc;.*$gdb_prompt 
> $"\
> {pass "step through execl call"}
>
> I think I might want to add one more lit test, because the LLVM suite didn't 
> catch a thinko that was the root cause of that last prolog weirdness.  But 
> this version is totally ready for review.

Thanks - that seems to cover Google's internal gdb test execution as well. 
Appreciate the test patch!

I haven't looked closely, but I take it this one test modification seems 
reasonable to you? I guess we're stepping into some subexpressions on a 
function call that we previously didn't? (they didn't have any instructions 
attributed to them until now, but it's not unreasonable that they have them 
attributed - for materializing constants to pass to a function call when the 
function call/constants are split over multiple lines, etc)


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

https://reviews.llvm.org/D91734

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

[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D91734#2482280 , @probinson wrote:

> In D91734#2480930 , @dblaikie wrote:
>
>> I haven't looked closely, but I take it this one test modification seems 
>> reasonable to you? I guess we're stepping into some subexpressions on a 
>> function call that we previously didn't? (they didn't have any instructions 
>> attributed to them until now, but it's not unreasonable that they have them 
>> attributed - for materializing constants to pass to a function call when the 
>> function call/constants are split over multiple lines, etc)
>
> Exactly.  In each case, the test is trying to "next" over a function call; 
> gcc attributes all parameter evaluation to the function name, while clang 
> will attribute each parameter to its own location.  And when the parameters 
> span multiple source lines, the is_stmt heuristic kicks in, so we stop on 
> each line with actual parameters.
>
> This is not ideal IMO; I'd rather be more principled about (a) stop at every 
> parameter, or (b) stop at no parameters.  But without reworking how we do 
> is_stmt, I think fiddling the test to do enough single-steps to actually get 
> past the function call is okay.

I get that it's a bit unreliable for the user - though GCC's approach isn't so 
different. If any of the parameter evaluations are themselves function calls, I 
think it does still multistep as well.

Actually, it seems it does not. Somewhere back around GCC 6 it did what we're 
doing, but since about 8 at least, no matter what the expressions (at least I 
tested for + and function call) it attributes all the instructions to the 
function call line - even the backtrace is misleading - I don't think it's 
better, just different.

But, yes, we could possibly do better with more strategic is_stmt, but that's a 
big/complex piece of work to tackle.


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

https://reviews.llvm.org/D91734

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

[Lldb-commits] [PATCH] D78978: [LLDB] Add support for WebAssembly debugging

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D78978#2483327 , @paolosev wrote:

> In D78978#2481358 , @vwzm228 wrote:
>
>> Is there any progress about such patch and D78801 
>> ？
>>
>> I have implemented the debugging feature in our Wasm VM based on  
>> https://reviews.llvm.org/D78801, and it already work to attach, set 
>> breakpoint, step, show variable value, backtrace...
>>
>> I am not sure if I need to change LLDB part to this one, or keep using 
>> D78801 .
>>
>> But if both patches wont be merged, I have to maintain a private LLDB 
>> version
>
> Unfortunately I have not received any more feedback for this patch, or the 
> alternative version D78801 , so I assumed it 
> won't move forward :(
> Meanwhile, I have created a personal fork 
> (https://github.com/paolosevMSFT/llvm-project/tree/WasmDbg).
>
> I still think that it would be very useful to add support for Wasm debugging 
> to LLDB. Especially in scenarios where Wasm is not used as part of a web app, 
> but server side (node.js) or running on micro runtime for IoT devices.
> I know that there is the problem that LLDB does not support segmented address 
> spaces, and so this patch requires a couple of tiny but smelly changes to 
> core code.
> From the mailing list I seem to remember that somebody was working to add 
> support for segmented addresses, but I don't know what is the current state 
> of the initiative.
>
> I'd be happy to keep working on this, please let me know what I could do to 
> progress toward an acceptable solution.

Usually the thing is to ping the review thread at most weekly & maybe search 
around for specific reviewers to ask if you're met with a lot of silence.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78978

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

[Lldb-commits] [PATCH] D94063: lldb: Add support for DW_AT_ranges on DW_TAG_subprograms

[David Blaikie via Phabricator via lldb-commits]

dblaikie updated this revision to Diff 315035.
dblaikie added a comment.

Update test to avoid running the program


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94063

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
  lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
  lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test

Index: lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
@@ -0,0 +1,17 @@
+# REQUIRES: x86
+# RUN: %clang -target x86_64-pc-linux -g -O0 %S/Inputs/subprogram_ranges.s -o %t.out
+# RUN: %lldb -b -s %s %t.out 2>&1 | FileCheck %s
+
+# Test breaking on symbols and printing variables when a DW_TAG_subprogram uses
+# DW_AT_ranges instead of DW_AT_low_pc/DW_AT_high_pc.  While the assembly here
+# is a bit unrealistic - it's a single-entry range using DWARFv4 which isn't
+# useful for anything (a single-entry range with DWARFv5 can reduce address
+# relocations, and multi-entry ranges can be used for function sections), but
+# it's the simplest thing to test. If anyone's updating this test at some
+# point, feel free to replace it with another equivalent test if it's
+# especially useful, but don't dismiss it as pointless just because it's a bit
+# weird.
+
+b main
+# CHECK: (lldb) b main
+# CHECK-NEXT: Breakpoint 1: where = subprogram_ranges.test.tmp.out`main + 6 at main.c:2:7
Index: lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
@@ -0,0 +1,159 @@
+	.text
+	.file	"main.c"
+	.globl	main# -- Begin function main
+	.p2align	4, 0x90
+	.type	main,@function
+main:   # @main
+.Lfunc_begin0:
+	.file	1 "/usr/local/google/home/blaikie/dev/scratch" "main.c"
+	.loc	1 1 0   # main.c:1:0
+	.cfi_startproc
+# %bb.0:# %entry
+	pushq	%rbp
+	.cfi_def_cfa_offset 16
+	.cfi_offset %rbp, -16
+	movq	%rsp, %rbp
+	.cfi_def_cfa_register %rbp
+	xorl	%eax, %eax
+.Ltmp0:
+	.loc	1 2 7 prologue_end  # main.c:2:7
+	movl	$3, -4(%rbp)
+	.loc	1 3 1   # main.c:3:1
+	popq	%rbp
+	.cfi_def_cfa %rsp, 8
+	retq
+.Ltmp1:
+.Lfunc_end0:
+	.size	main, .Lfunc_end0-main
+	.cfi_endproc
+# -- End function
+	.section	.debug_abbrev,"",@progbits
+	.byte	1   # Abbreviation Code
+	.byte	17  # DW_TAG_compile_unit
+	.byte	1   # DW_CHILDREN_yes
+	.byte	37  # DW_AT_producer
+	.byte	14  # DW_FORM_strp
+	.byte	19  # DW_AT_language
+	.byte	5   # DW_FORM_data2
+	.byte	3   # DW_AT_name
+	.byte	14  # DW_FORM_strp
+	.byte	16  # DW_AT_stmt_list
+	.byte	23  # DW_FORM_sec_offset
+	.byte	27  # DW_AT_comp_dir
+	.byte	14  # DW_FORM_strp
+	.byte	17  # DW_AT_low_pc
+	.byte	1   # DW_FORM_addr
+	.byte	18  # DW_AT_high_pc
+	.byte	6   # DW_FORM_data4
+	.byte	0   # EOM(1)
+	.byte	0   # EOM(2)
+	.byte	2   # Abbreviation Code
+	.byte	46  # DW_TAG_subprogram
+	.byte	1   # DW_CHILDREN_yes
+	.byte	85  # DW_AT_ranges
+	.byte	23  # DW_FORM_sec_offset
+	.byte	64  # DW_AT_frame_base
+	.byte	24  # DW_FORM_exprloc
+	.byte	3   # DW_AT_name
+	.byte	14  # DW_FORM_strp
+	.byte	58  # DW_AT_decl_file
+	.byte	11  # DW_FORM_data1
+	.byte	59  # DW_AT_decl_line
+	.byte	11  # DW_FORM_data1
+	.byte	73  # DW_AT_type
+	.byte	19  # DW_FORM_ref4
+	.byte	63  # DW_AT_external
+	.byte	25  # DW_FORM_flag_present
+	.byte	0   # EOM(1)
+	.byte	0   # EOM(2)
+	.byte	3   # Abbreviation Code
+	.byte	52  # DW_TAG_variable
+	.byte	0   

[Lldb-commits] [PATCH] D94063: lldb: Add support for DW_AT_ranges on DW_TAG_subprograms

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D94063#2481867 , @labath wrote:

> The fix is good, but the test could be improved.

Yeah - haven't written lldb patches before so totally open to suggestions on 
the testing front for sure. Thanks!

> Combining assembly input with running the inferior effectively limits the 
> test to a single platform (assembly is not portable, and running requires asm 
> to match the host).

If it's better to write it using C++ source and custom clang flags I can do 
that instead (it'll be an -mllvm flag - looks like there's one other test that 
does that: `lldb/test/API/lang/objc/forward-decl/TestForwardDecl.py:
dict(CFLAGS_EXTRAS="-dwarf-version=5 -mllvm -accel-tables=Dwarf"))`) - means 
the test will be a bit more convoluted to tickle the subprogram ranges, but not 
much - just takes two functions+function-sections.

> AFAICT, we don't actually need to run the binary to test this fix -- checking 
> just the "breakpoint set" command should suffice (if you want to be more 
> explicit in what is being checked, you can also run "breakpoint list -v" and 
> test that). Then you'd just need to replace `%clang_host` with `%clang 
> -target x86_64-pc-linux` (or something) and `UNSUPPORTED: system-windows` 
> with `REQUIRES: x86`.

Yeah, I was hoping to setup the test for testing both these things in a uniform 
way, but if best practice is to test them with different mechanisms I'm OK with 
that.

> I'll write about variable printing in the other review.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94063

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

[Lldb-commits] [PATCH] D94064: lldb: Add support for printing variables with DW_AT_ranges on DW_TAG_subprograms

[David Blaikie via Phabricator via lldb-commits]

dblaikie updated this revision to Diff 315036.
dblaikie added a comment.

Use image lookup to make test runnable without executing the test code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94064

Files:
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test


Index: lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
===
--- lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
+++ lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
@@ -15,3 +15,7 @@
 b main
 # CHECK: (lldb) b main
 # CHECK-NEXT: Breakpoint 1: where = subprogram_ranges.test.tmp.out`main + 6 at 
main.c:2:7
+
+image lookup -v -s main
+# CHECK: 1 symbols match 'main'
+# CHECK:  Variable: {{.*}}, name = "var", type = "int", {{.*}}, decl = main.c:2
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -3040,8 +3040,13 @@
 if (sc.function) {
   DWARFDIE function_die = GetDIE(sc.function->GetID());
 
-  const dw_addr_t func_lo_pc = function_die.GetAttributeValueAsAddress(
+  dw_addr_t func_lo_pc = function_die.GetAttributeValueAsAddress(
   DW_AT_low_pc, LLDB_INVALID_ADDRESS);
+  DWARFFormValue form_value;
+  if (func_lo_pc == LLDB_INVALID_ADDRESS &&
+  function_die.GetDIE()->GetAttributeValue(function_die.GetCU(),
+   DW_AT_ranges, form_value))
+func_lo_pc = 0;
   if (func_lo_pc != LLDB_INVALID_ADDRESS) {
 const size_t num_variables = ParseVariables(
 sc, function_die.GetFirstChild(), func_lo_pc, true, true);


Index: lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
===
--- lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
+++ lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
@@ -15,3 +15,7 @@
 b main
 # CHECK: (lldb) b main
 # CHECK-NEXT: Breakpoint 1: where = subprogram_ranges.test.tmp.out`main + 6 at main.c:2:7
+
+image lookup -v -s main
+# CHECK: 1 symbols match 'main'
+# CHECK:  Variable: {{.*}}, name = "var", type = "int", {{.*}}, decl = main.c:2
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -3040,8 +3040,13 @@
 if (sc.function) {
   DWARFDIE function_die = GetDIE(sc.function->GetID());
 
-  const dw_addr_t func_lo_pc = function_die.GetAttributeValueAsAddress(
+  dw_addr_t func_lo_pc = function_die.GetAttributeValueAsAddress(
   DW_AT_low_pc, LLDB_INVALID_ADDRESS);
+  DWARFFormValue form_value;
+  if (func_lo_pc == LLDB_INVALID_ADDRESS &&
+  function_die.GetDIE()->GetAttributeValue(function_die.GetCU(),
+   DW_AT_ranges, form_value))
+func_lo_pc = 0;
   if (func_lo_pc != LLDB_INVALID_ADDRESS) {
 const size_t num_variables = ParseVariables(
 sc, function_die.GetFirstChild(), func_lo_pc, true, true);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D94064: lldb: Add support for printing variables with DW_AT_ranges on DW_TAG_subprograms

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D94064#2481925 , @labath wrote:

> I don't think that simply setting func_lo_pc to zero will be sufficient to 
> make this work. I would expect this to break in more complicated scenarios 
> (like, even if we just have two of these functions). I think the only reason 
> it works in this case is because for this particular function, it's base 
> address (relative to the CU base) *is* zero.

I certainly don't have high confidence in the change, to be sure - but I think 
it's a bit more robust than that.

Here's at least one experiment where a function is at a non-zero offset 
relative to the CU:

  $ cat test.cpp
  void stop() {
  }
  void f1() {
int i = 7;
stop();
  }
  int main() {
int j = 12;
f1();
stop();
  }
  $ $ clang++-tot test.cpp -gdwarf-5 -mllvm -always-use-ranges-in-v5=Enable && 
~/dev/llvm/build/default/bin/lldb ./a.out
  (lldb) target create "./a.out"
  Current executable set to '/usr/local/google/home/blaikie/dev/scratch/a.out' 
(x86_64).
  (lldb) r
  Process 1039251 launched: '/usr/local/google/home/blaikie/dev/scratch/a.out' 
(x86_64)
  Process 1039251 exited with status = 0 (0x) 
  (lldb) b stop
  Breakpoint 1: where = a.out`stop() + 4 at test.cpp:2:1, address = 
0x00401114
  (lldb) r
  Process 1039605 launched: '/usr/local/google/home/blaikie/dev/scratch/a.out' 
(x86_64)
  Process 1039605 stopped
  * thread #1, name = 'a.out', stop reason = breakpoint 1.1
  frame #0: 0x00401114 a.out`stop() at test.cpp:2:1
 1void stop() {
  -> 2}
 3void f1() {
 4  int i = 7;
 5  stop();
 6}
 7int main() {
  (lldb) up
  frame #1: 0x00401134 a.out`f1() at test.cpp:5:3
 2}
 3void f1() {
 4  int i = 7;
  -> 5  stop();
 6}
 7int main() {
 8  int j = 12;
  (lldb) p i
  (int) $0 = 7
  (lldb) c
  Process 1039605 resuming
  Process 1039605 stopped
  * thread #1, name = 'a.out', stop reason = breakpoint 1.1
  frame #0: 0x00401114 a.out`stop() at test.cpp:2:1
 1void stop() {
  -> 2}
 3void f1() {
 4  int i = 7;
 5  stop();
 6}
 7int main() {
  (lldb) up
  frame #1: 0x00401159 a.out`main at test.cpp:10:3
 7int main() {
 8  int j = 12;
 9  f1();
  -> 10 stop();
 11   }
  (lldb) p j
  (int) $1 = 12
  $ llvm-dwarfdump a.out
  .debug_info contents:
  0x: Compile Unit: length = 0x005f, format = DWARF32, version = 
0x0005, unit_type = DW_UT_compile, abbr_offset = 0x, addr_size = 0x08 (next 
unit at 0x0063)
  
  0x000c: DW_TAG_compile_unit
DW_AT_producer("clang version 12.0.0 
(g...@github.com:llvm/llvm-project.git 
13740c7d80e6c7b47506fd34cd2ea2a7b658cdfa)")
DW_AT_language(DW_LANG_C_plus_plus_14)
DW_AT_name("test.cpp")
DW_AT_str_offsets_base(0x0008)
DW_AT_stmt_list   (0x)
DW_AT_comp_dir("/usr/local/google/home/blaikie/dev/scratch")
DW_AT_low_pc  (0x00401110)
DW_AT_high_pc (0x00401161)
DW_AT_addr_base   (0x0008)
DW_AT_rnglists_base   (0x000c)
  
  0x0027:   DW_TAG_subprogram
  DW_AT_low_pc(0x00401110)
  DW_AT_high_pc   (0x00401116)
  DW_AT_frame_base(DW_OP_reg6 RBP)
  DW_AT_linkage_name  ("_Z4stopv")
  DW_AT_name  ("stop")
  DW_AT_decl_file 
("/usr/local/google/home/blaikie/dev/scratch/test.cpp")
  DW_AT_decl_line (1)
  DW_AT_external  (true)
  
  0x0033:   DW_TAG_subprogram
  DW_AT_ranges(indexed (0x0) rangelist = 0x0014
 [0x00401120, 0x0040113a))
  DW_AT_frame_base(DW_OP_reg6 RBP)
  DW_AT_linkage_name  ("_Z2f1v")
  DW_AT_name  ("f1")
  DW_AT_decl_file 
("/usr/local/google/home/blaikie/dev/scratch/test.cpp")
  DW_AT_decl_line (3)
  DW_AT_external  (true)
  
  0x003b: DW_TAG_variable
DW_AT_location(DW_OP_fbreg -4)
DW_AT_name("i")
DW_AT_decl_file   
("/usr/local/google/home/blaikie/dev/scratch/test.cpp")
DW_AT_decl_line   (4)
DW_AT_type(0x005e "int")
  
  0x0046: NULL
  
  0x0047:   DW_TAG_subprogram
  DW_AT_ranges(indexed (0x1) rangelist = 0x0018
 [0x00401140, 0x00401161))
  DW_AT_frame_base(DW_OP_reg6 RBP)
  DW_AT_name  ("main")
  DW_AT_decl_fi

[Lldb-commits] [PATCH] D94063: lldb: Add support for DW_AT_ranges on DW_TAG_subprograms

[David Blaikie via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG274afac9a17f: lldb: Add support for DW_AT_ranges on 
DW_TAG_subprograms (authored by dblaikie).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94063

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
  lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
  lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test

Index: lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
@@ -0,0 +1,17 @@
+# REQUIRES: x86
+# RUN: %clang -target x86_64-pc-linux -g -O0 %S/Inputs/subprogram_ranges.s -o %t.out
+# RUN: %lldb -b -s %s %t.out 2>&1 | FileCheck %s
+
+# Test breaking on symbols and printing variables when a DW_TAG_subprogram uses
+# DW_AT_ranges instead of DW_AT_low_pc/DW_AT_high_pc.  While the assembly here
+# is a bit unrealistic - it's a single-entry range using DWARFv4 which isn't
+# useful for anything (a single-entry range with DWARFv5 can reduce address
+# relocations, and multi-entry ranges can be used for function sections), but
+# it's the simplest thing to test. If anyone's updating this test at some
+# point, feel free to replace it with another equivalent test if it's
+# especially useful, but don't dismiss it as pointless just because it's a bit
+# weird.
+
+b main
+# CHECK: (lldb) b main
+# CHECK-NEXT: Breakpoint 1: where = subprogram_ranges.test.tmp.out`main + 6 at main.c:2:7
Index: lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
@@ -0,0 +1,159 @@
+	.text
+	.file	"main.c"
+	.globl	main# -- Begin function main
+	.p2align	4, 0x90
+	.type	main,@function
+main:   # @main
+.Lfunc_begin0:
+	.file	1 "/usr/local/google/home/blaikie/dev/scratch" "main.c"
+	.loc	1 1 0   # main.c:1:0
+	.cfi_startproc
+# %bb.0:# %entry
+	pushq	%rbp
+	.cfi_def_cfa_offset 16
+	.cfi_offset %rbp, -16
+	movq	%rsp, %rbp
+	.cfi_def_cfa_register %rbp
+	xorl	%eax, %eax
+.Ltmp0:
+	.loc	1 2 7 prologue_end  # main.c:2:7
+	movl	$3, -4(%rbp)
+	.loc	1 3 1   # main.c:3:1
+	popq	%rbp
+	.cfi_def_cfa %rsp, 8
+	retq
+.Ltmp1:
+.Lfunc_end0:
+	.size	main, .Lfunc_end0-main
+	.cfi_endproc
+# -- End function
+	.section	.debug_abbrev,"",@progbits
+	.byte	1   # Abbreviation Code
+	.byte	17  # DW_TAG_compile_unit
+	.byte	1   # DW_CHILDREN_yes
+	.byte	37  # DW_AT_producer
+	.byte	14  # DW_FORM_strp
+	.byte	19  # DW_AT_language
+	.byte	5   # DW_FORM_data2
+	.byte	3   # DW_AT_name
+	.byte	14  # DW_FORM_strp
+	.byte	16  # DW_AT_stmt_list
+	.byte	23  # DW_FORM_sec_offset
+	.byte	27  # DW_AT_comp_dir
+	.byte	14  # DW_FORM_strp
+	.byte	17  # DW_AT_low_pc
+	.byte	1   # DW_FORM_addr
+	.byte	18  # DW_AT_high_pc
+	.byte	6   # DW_FORM_data4
+	.byte	0   # EOM(1)
+	.byte	0   # EOM(2)
+	.byte	2   # Abbreviation Code
+	.byte	46  # DW_TAG_subprogram
+	.byte	1   # DW_CHILDREN_yes
+	.byte	85  # DW_AT_ranges
+	.byte	23  # DW_FORM_sec_offset
+	.byte	64  # DW_AT_frame_base
+	.byte	24  # DW_FORM_exprloc
+	.byte	3   # DW_AT_name
+	.byte	14  # DW_FORM_strp
+	.byte	58  # DW_AT_decl_file
+	.byte	11  # DW_FORM_data1
+	.byte	59  # DW_AT_decl_line
+	.byte	11  # DW_FORM_data1
+	.byte	73  # DW_AT_type
+	.byte	19  # DW_FORM_ref4
+	.byte	63  # DW_AT_external
+	.byte	25  # DW_FORM_flag_present
+	.byte	0   # EOM(1)
+	.byte	0   # EOM(2)
+	.byte	3

[Lldb-commits] [PATCH] D94063: lldb: Add support for DW_AT_ranges on DW_TAG_subprograms

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D94063#2485271 , @labath wrote:

> In D94063#2483546 , @dblaikie wrote:
>
>> If it's better to write it using C++ source and custom clang flags I can do 
>> that instead (it'll be an -mllvm flag - looks like there's one other test 
>> that does that: `lldb/test/API/lang/objc/forward-decl/TestForwardDecl.py:
>> dict(CFLAGS_EXTRAS="-dwarf-version=5 -mllvm -accel-tables=Dwarf"))`) 
>> - means the test will be a bit more convoluted to tickle the subprogram 
>> ranges, but not much - just takes two functions+function-sections.
>
> I certainly wouldn't want to drop the existing test.

Ah, what's the tradeoff between the different test types here?

> However, it could be useful to have c++ test too. This one could feature a 
> more complicated executable, and be more open-ended/exploratory and test 
> end-to-end functionality (including compiler integration), instead of a 
> targeted "did we parse DW_AT_ranges correctly" regression test. Then this 
> test could go into the `API` test category, as we have the ability to run 
> those kinds of tests against different compilers.

Does this include support for custom compiler flags (it'd currently take a 
non-official/internal-only llvm flag to create the DW_AT_ranges on a subprogram 
that I have in mind, for instance)?

> However, all of that is strictly optional.

I'll consider it for a separate commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94063

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

[Lldb-commits] [PATCH] D94063: lldb: Add support for DW_AT_ranges on DW_TAG_subprograms

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D94063#2492450 , @DavidSpickett 
wrote:

> Do you have python enabled in the build? 
> (https://lldb.llvm.org/resources/build.html#preliminaries)
>
> The cmake option LLDB_ENABLE_PYTHON defaults to Auto which has tripped me up 
> in the past. If you set it to YES then you'll get a build error if it fails 
> to find a suitable Python instead of silently disabling it.

Thanks for the hint! Yep, seems I didn't have the python3-dev package installed 
(took a while to stare at the error messages to guess/understand that that's 
what I was missing)

That got a few of these tests running, but still a significant chunk are 
"unsupported" due to (well, at least some are due to this - I'm guessing quite 
a few are due to this):

  lldb version 12.0.0 (g...@github.com:llvm/llvm-project.git revision 
d49974f9c98ebce5a679eced9f27add138b881fa)
clang revision d49974f9c98ebce5a679eced9f27add138b881fa
llvm revision d49974f9c98ebce5a679eced9f27add138b881fa
  Libc++ tests will not be run because: Compiling with -stdlib=libc++ fails 
with the error: :1:10: fatal error: 'algorithm' file not found
  #include 
   ^~~
  1 error generated.
  
  Skipping following debug info categories: ['dsym', 'gmodules']

But I do have libc++ and libc++abi listed as projects to build in my cmake 
config - but I guess because the just-built clang and just-built libc++ aren't 
installed, clang can't find libc++ so these tests fail/get marked as 
unsupported.

But how do these tests work for other people? Do they ever run in a plain build 
like this? It seems like they should/that would be important.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94063

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

[Lldb-commits] [PATCH] D94888: [lldb] Add -Wl, -rpath to make tests run with fresh built libc++

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

(Works for me, FWIW - takes my local run down from 42 failures in 
check-lldb-api to 1 (a timeout))

Out of curiousity did you find a way to get lldb-test to print out the 
stdout/stderr of the underlying lldb process to see what error messages it was 
producing that demonstrated this failure? I was trying to figure out how to do 
that, and it seems like a good thing to be able to do for this issue or others 
down the way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94888

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

[Lldb-commits] [PATCH] D94064: lldb: Add support for printing variables with DW_AT_ranges on DW_TAG_subprograms

[David Blaikie via Phabricator via lldb-commits]

dblaikie updated this revision to Diff 318654.
dblaikie added a comment.

Retrieve the lowest address in the address ranges, rather than just hardcoding 0


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94064

Files:
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
  lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test

Index: lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
===
--- lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
+++ lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
@@ -16,4 +16,8 @@
 
 b main
 # CHECK: (lldb) b main
-# CHECK-NEXT: Breakpoint 1: where = {{.*}}`main + 6 at main.c:2:7
+# CHECK-NEXT: Breakpoint 1: where = {{.*}}`main at subprogram_ranges.c:4:5
+
+image lookup -v -s main
+# CHECK: 1 symbols match 'main'
+# CHECK:  Variable: {{.*}}, name = "var", type = "int", {{.*}}, decl = subprogram_ranges.c:3
Index: lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
===
--- lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
+++ lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
@@ -1,32 +1,53 @@
 	.text
-	.file	"main.c"
+	.file	"subprogram_ranges.c"
 	.globl	main# -- Begin function main
 	.p2align	4, 0x90
 	.type	main,@function
 main:   # @main
 .Lfunc_begin0:
-	.file	1 "/usr/local/google/home/blaikie/dev/scratch" "main.c"
-	.loc	1 1 0   # main.c:1:0
+	.file	1 "/usr/local/google/home/blaikie/dev/scratch" "subprogram_ranges.c"
+	.loc	1 2 0   # subprogram_ranges.c:2:0
 	.cfi_startproc
 # %bb.0:# %entry
-	pushq	%rbp
-	.cfi_def_cfa_offset 16
-	.cfi_offset %rbp, -16
-	movq	%rsp, %rbp
-	.cfi_def_cfa_register %rbp
-	xorl	%eax, %eax
+	#DEBUG_VALUE: main:var <- 3
+	.loc	1 4 5 prologue_end  # subprogram_ranges.c:4:5
+	movl	$1, i(%rip)
 .Ltmp0:
-	.loc	1 2 7 prologue_end  # main.c:2:7
-	movl	$3, -4(%rbp)
-	.loc	1 3 1   # main.c:3:1
-	popq	%rbp
-	.cfi_def_cfa %rsp, 8
+	#DEBUG_VALUE: main:var <- 5
+	.loc	1 6 5   # subprogram_ranges.c:6:5
+	movl	$2, i(%rip)
+	.loc	1 7 1   # subprogram_ranges.c:7:1
+	xorl	%eax, %eax
 	retq
 .Ltmp1:
 .Lfunc_end0:
 	.size	main, .Lfunc_end0-main
 	.cfi_endproc
 # -- End function
+	.type	i,@object   # @i
+	.bss
+	.globl	i
+	.p2align	2
+i:
+	.long	0   # 0x0
+	.size	i, 4
+
+	.section	.debug_loc,"",@progbits
+.Ldebug_loc0:
+	.quad	.Lfunc_begin0-.Lfunc_begin0
+	.quad	.Ltmp0-.Lfunc_begin0
+	.short	3   # Loc expr size
+	.byte	17  # DW_OP_consts
+	.byte	3   # 3
+	.byte	159 # DW_OP_stack_value
+	.quad	.Ltmp0-.Lfunc_begin0
+	.quad	.Lfunc_end0-.Lfunc_begin0
+	.short	3   # Loc expr size
+	.byte	17  # DW_OP_consts
+	.byte	5   # 5
+	.byte	159 # DW_OP_stack_value
+	.quad	0
+	.quad	0
 	.section	.debug_abbrev,"",@progbits
 	.byte	1   # Abbreviation Code
 	.byte	17  # DW_TAG_compile_unit
@@ -54,6 +75,8 @@
 	.byte	23  # DW_FORM_sec_offset
 	.byte	64  # DW_AT_frame_base
 	.byte	24  # DW_FORM_exprloc
+	.ascii	"\227B" # DW_AT_GNU_all_call_sites
+	.byte	25  # DW_FORM_flag_present
 	.byte	3   # DW_AT_name
 	.byte	14  # DW_FORM_strp
 	.byte	58  # DW_AT_decl_file
@@ -70,7 +93,7 @@
 	.byte	52  # DW_TAG_variable
 	.byte	0   # DW_CHILDREN_no
 	.byte	2   # DW_AT_location
-	.byte	24  # DW_FORM_exprloc
+	.byte	23  # DW_FORM_sec_offset
 	.byte	3   # DW_AT_name
 	.byte	14  # DW_FORM_strp
 	.byte	58  # DW_AT_decl_file
@@ -100,7 +123,7 @@
 	.short	4   # DWARF version number
 	.long	.debug_abbrev   # Offset Into Abbrev. Section
 	.byte	8   # Address Size (in bytes)
-	.byte	1   # Abbrev [1] 0xb:0x47 DW_TAG_compile_unit
+	.byte	1   # Abbrev [1] 0xb:0x48 DW_TAG_compile_unit
 	.long	.Linfo_string0  # DW_AT_producer
 	

[Lldb-commits] [PATCH] D94064: lldb: Add support for printing variables with DW_AT_ranges on DW_TAG_subprograms

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D94064#2485222 , @labath wrote:

> In D94064#2483578 , @dblaikie wrote:
>
>> In D94064#2481925 , @labath wrote:
>>
>>> I don't think that simply setting func_lo_pc to zero will be sufficient to 
>>> make this work. I would expect this to break in more complicated scenarios 
>>> (like, even if we just have two of these functions). I think the only 
>>> reason it works in this case is because for this particular function, it's 
>>> base address (relative to the CU base) *is* zero.
>>
>> I certainly don't have high confidence in the change, to be sure - but I 
>> think it's a bit more robust than that.
>
> Yes, it seems it is, though just by a bit. :)
>
> The thing which is missing is the location list on the variable DIE -- 
> without it, it does not matter which address gets put here (so long as it 
> does not trigger the assertion), as the value is totally unused. The 
> necessity of the location list is kind of obvious from my description of the 
> problem, though even I did not realize when I was writing it that this 
> requires a more elaborate test case.
>
> Adjusting your test case to produce a location list for `i` I got this 
> (probably not minimal) snippet (add -O1 to CFLAGS):
>
>   #define optnone __attribute__((optnone))
>   #define noinline __attribute__((noinline))
>   
>   void optnone noinline stop() {}
>   void optnone noinline consume(int i) {}
>   void noinline f1(int x) {
> int i = x;
> stop();
> consume(i);
> i = 5;
> consume(i);
>   }
>   int main() {
> int j = 12;
> f1(j);
> stop();
>   }
>
> Here lldb fails to print the value of `i`, but if I rearrange the code so 
> that the `f1` functions comes first, the value is printed correctly:
>
>   #define optnone __attribute__((optnone))
>   #define noinline __attribute__((noinline))
>   
>   void optnone noinline stop();
>   void optnone noinline consume(int i);
>   void noinline f1(int x) {
> int i = x;
> stop();
> consume(i);
> i = 5;
> consume(i);
>   }
>   void optnone noinline stop() {}
>   void optnone noinline consume(int i) {}
>   int main() {
> int j = 12;
> f1(j);
> stop();
>   }

Ah, thanks - think I figured out a representative test & understand better.
I ended up with this:

  __attribute__((nodebug)) volatile int i;
  int main() {
int var = 3;
i = 1;
var = 5;
i = 2;
  }

By using volatile writes, I was able to get the 'var' variable live range to 
start at the start of the function (so that `image lookup -v -s main` would 
render the "var" variable since it's now live at the very start of the function 
(rather than only after the push instruction)). And use nodebug to reduce the 
DWARF since the 'i' isn't interesting.

Indeed without the fix you suggested to use "lowest address" rather than zero, 
this test above would/was failing (running the image lookup command would not 
show the "var" variable).

Also, I figured out how to run the API tests, and that showed a bunch more 
failures when using ranges-everywhere (actually a more aggressive version of 
ranges-everywhere - using it no matter the DWARF version and even when it 
wouldn't reduce the address pool size (eg: using ranges on a subprogram even 
when low_pc is an address already in the pool (such as for the start of a CU 
range))) - and with the change to use the lowest address of the ranges, all 
those failures now go away.

So *fingers crossed* this is ready.

>>> The purpose of func_lo_pc is pretty weird, but it's essentially used for 
>>> runtime relocation of location lists within the function -- we take the 
>>> static "base" of the function, and the runtime "base", and the difference 
>>> between the two produces the load bias. Applying the same bias to all 
>>> variable location lists "relocates" the variables. (The reason this is so 
>>> convoluted is reading location lists straight from (unrelocated) .o files 
>>> on mac. I seriously considered changing this when working on 
>>> debug_rnglists, but it turned out it wasn't really necessary, so I let it 
>>> go.)
>>
>> Yep, I figured a bunch of this was for DWARF in unrelocated MachO files - 
>> and that they wouldn't be able to/want to use Split DWARF or this feature 
>> (which is particularly relevant to Split DWARF). Does any of this logic 
>> apply outside that unrelocated MachO scenario?
>
> Yes, this code is used everywhere, though it's role is less important (and 
> one that could be implemented in an easier manner, were it not for MachO) -- 
> it adjusts the variable address for ASLR, i.e., the variable/function being 
> loaded at a different address than what the static debug info says.

Ah, good to know!

>>> LLDB's representation of a function (lldb_private::Function) assumes that 
>>> all functions will be contiguous (Function::GetAddressRange returns a 
>>> single range

[Lldb-commits] [PATCH] D94064: lldb: Add support for printing variables with DW_AT_ranges on DW_TAG_subprograms

[David Blaikie via Phabricator via lldb-commits]

dblaikie updated this revision to Diff 318711.
dblaikie added a comment.

Include some more details about/in the test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94064

Files:
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
  lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test

Index: lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
===
--- lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
+++ lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
@@ -14,6 +14,26 @@
 # especially useful, but don't dismiss it as pointless just because it's a bit
 # weird.
 
+# * Using volatile writes to create instructions the location may be valid over
+# * Using two values for the variable so it is described by a location list,
+#   not a single location description
+# * Not using function calls, so that the function has no frame pointer
+#   initialization/no prologue instructions, so the location of "var" is valid
+#   at the start of the function, so 'image lookup -v -s main' will include it.
+#
+# Source:
+# __attribute__((nodebug)) volatile int i;
+# int main() {
+#   int var = 3;
+#   i = 1;
+#   var = 5;
+#   i = 2;
+# }
+
 b main
 # CHECK: (lldb) b main
-# CHECK-NEXT: Breakpoint 1: where = {{.*}}`main + 6 at main.c:2:7
+# CHECK-NEXT: Breakpoint 1: where = {{.*}}`main at subprogram_ranges.c:4:5
+
+image lookup -v -s main
+# CHECK: 1 symbols match 'main'
+# CHECK:  Variable: {{.*}}, name = "var", type = "int", {{.*}}, decl = subprogram_ranges.c:3
Index: lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
===
--- lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
+++ lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
@@ -1,32 +1,53 @@
 	.text
-	.file	"main.c"
+	.file	"subprogram_ranges.c"
 	.globl	main# -- Begin function main
 	.p2align	4, 0x90
 	.type	main,@function
 main:   # @main
 .Lfunc_begin0:
-	.file	1 "/usr/local/google/home/blaikie/dev/scratch" "main.c"
-	.loc	1 1 0   # main.c:1:0
+	.file	1 "/usr/local/google/home/blaikie/dev/scratch" "subprogram_ranges.c"
+	.loc	1 2 0   # subprogram_ranges.c:2:0
 	.cfi_startproc
 # %bb.0:# %entry
-	pushq	%rbp
-	.cfi_def_cfa_offset 16
-	.cfi_offset %rbp, -16
-	movq	%rsp, %rbp
-	.cfi_def_cfa_register %rbp
-	xorl	%eax, %eax
+	#DEBUG_VALUE: main:var <- 3
+	.loc	1 4 5 prologue_end  # subprogram_ranges.c:4:5
+	movl	$1, i(%rip)
 .Ltmp0:
-	.loc	1 2 7 prologue_end  # main.c:2:7
-	movl	$3, -4(%rbp)
-	.loc	1 3 1   # main.c:3:1
-	popq	%rbp
-	.cfi_def_cfa %rsp, 8
+	#DEBUG_VALUE: main:var <- 5
+	.loc	1 6 5   # subprogram_ranges.c:6:5
+	movl	$2, i(%rip)
+	.loc	1 7 1   # subprogram_ranges.c:7:1
+	xorl	%eax, %eax
 	retq
 .Ltmp1:
 .Lfunc_end0:
 	.size	main, .Lfunc_end0-main
 	.cfi_endproc
 # -- End function
+	.type	i,@object   # @i
+	.bss
+	.globl	i
+	.p2align	2
+i:
+	.long	0   # 0x0
+	.size	i, 4
+
+	.section	.debug_loc,"",@progbits
+.Ldebug_loc0:
+	.quad	.Lfunc_begin0-.Lfunc_begin0
+	.quad	.Ltmp0-.Lfunc_begin0
+	.short	3   # Loc expr size
+	.byte	17  # DW_OP_consts
+	.byte	3   # 3
+	.byte	159 # DW_OP_stack_value
+	.quad	.Ltmp0-.Lfunc_begin0
+	.quad	.Lfunc_end0-.Lfunc_begin0
+	.short	3   # Loc expr size
+	.byte	17  # DW_OP_consts
+	.byte	5   # 5
+	.byte	159 # DW_OP_stack_value
+	.quad	0
+	.quad	0
 	.section	.debug_abbrev,"",@progbits
 	.byte	1   # Abbreviation Code
 	.byte	17  # DW_TAG_compile_unit
@@ -54,6 +75,8 @@
 	.byte	23  # DW_FORM_sec_offset
 	.byte	64  # DW_AT_frame_base
 	.byte	24  # DW_FORM_exprloc
+	.ascii	"\227B" # DW_AT_GNU_all_call_sites
+	.byte	25  # DW_FORM_flag_present
 	.byte	3   # DW_AT_name
 	.byte	14  # DW_FORM_strp
 	.byte	58  # DW_AT_decl_file
@@ -70,7 +93,7 @@
 	.byte	52  # DW_TAG_variable
 	.byte	0   # DW_CHILDREN_no
 	.byte	2   # DW_AT_location
-	.byte	24  # DW_FORM_exprloc
+	.byte	23  # DW_FO

[Lldb-commits] [PATCH] D94064: lldb: Add support for printing variables with DW_AT_ranges on DW_TAG_subprograms

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test:22
+#   initialization/no prologue instructions, so the location of "var" is valid
+#   at the start of the function, so 'image lookup -v -s main' will include it.
+#

labath wrote:
> btw, the way I've dealt with this in the past is to introduce an additional 
> label in the assembly (`look_me_up:`), and then give that to the image lookup 
> command.
Oh, makes sense - will certainly keep that in mind! 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94064

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

[Lldb-commits] [PATCH] D94064: lldb: Add support for printing variables with DW_AT_ranges on DW_TAG_subprograms

[David Blaikie via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG78d41a1295d9: lldb: Add support for printing variables with 
DW_AT_ranges on DW_TAG_subprograms (authored by dblaikie).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94064

Files:
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
  lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test

Index: lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
===
--- lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
+++ lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
@@ -14,6 +14,26 @@
 # especially useful, but don't dismiss it as pointless just because it's a bit
 # weird.
 
+# * Using volatile writes to create instructions the location may be valid over
+# * Using two values for the variable so it is described by a location list,
+#   not a single location description
+# * Not using function calls, so that the function has no frame pointer
+#   initialization/no prologue instructions, so the location of "var" is valid
+#   at the start of the function, so 'image lookup -v -s main' will include it.
+#
+# Source:
+# __attribute__((nodebug)) volatile int i;
+# int main() {
+#   int var = 3;
+#   i = 1;
+#   var = 5;
+#   i = 2;
+# }
+
 b main
 # CHECK: (lldb) b main
-# CHECK-NEXT: Breakpoint 1: where = {{.*}}`main + 6 at main.c:2:7
+# CHECK-NEXT: Breakpoint 1: where = {{.*}}`main at subprogram_ranges.c:4:5
+
+image lookup -v -s main
+# CHECK: 1 symbols match 'main'
+# CHECK:  Variable: {{.*}}, name = "var", type = "int", {{.*}}, decl = subprogram_ranges.c:3
Index: lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
===
--- lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
+++ lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
@@ -1,32 +1,53 @@
 	.text
-	.file	"main.c"
+	.file	"subprogram_ranges.c"
 	.globl	main# -- Begin function main
 	.p2align	4, 0x90
 	.type	main,@function
 main:   # @main
 .Lfunc_begin0:
-	.file	1 "/usr/local/google/home/blaikie/dev/scratch" "main.c"
-	.loc	1 1 0   # main.c:1:0
+	.file	1 "/usr/local/google/home/blaikie/dev/scratch" "subprogram_ranges.c"
+	.loc	1 2 0   # subprogram_ranges.c:2:0
 	.cfi_startproc
 # %bb.0:# %entry
-	pushq	%rbp
-	.cfi_def_cfa_offset 16
-	.cfi_offset %rbp, -16
-	movq	%rsp, %rbp
-	.cfi_def_cfa_register %rbp
-	xorl	%eax, %eax
+	#DEBUG_VALUE: main:var <- 3
+	.loc	1 4 5 prologue_end  # subprogram_ranges.c:4:5
+	movl	$1, i(%rip)
 .Ltmp0:
-	.loc	1 2 7 prologue_end  # main.c:2:7
-	movl	$3, -4(%rbp)
-	.loc	1 3 1   # main.c:3:1
-	popq	%rbp
-	.cfi_def_cfa %rsp, 8
+	#DEBUG_VALUE: main:var <- 5
+	.loc	1 6 5   # subprogram_ranges.c:6:5
+	movl	$2, i(%rip)
+	.loc	1 7 1   # subprogram_ranges.c:7:1
+	xorl	%eax, %eax
 	retq
 .Ltmp1:
 .Lfunc_end0:
 	.size	main, .Lfunc_end0-main
 	.cfi_endproc
 # -- End function
+	.type	i,@object   # @i
+	.bss
+	.globl	i
+	.p2align	2
+i:
+	.long	0   # 0x0
+	.size	i, 4
+
+	.section	.debug_loc,"",@progbits
+.Ldebug_loc0:
+	.quad	.Lfunc_begin0-.Lfunc_begin0
+	.quad	.Ltmp0-.Lfunc_begin0
+	.short	3   # Loc expr size
+	.byte	17  # DW_OP_consts
+	.byte	3   # 3
+	.byte	159 # DW_OP_stack_value
+	.quad	.Ltmp0-.Lfunc_begin0
+	.quad	.Lfunc_end0-.Lfunc_begin0
+	.short	3   # Loc expr size
+	.byte	17  # DW_OP_consts
+	.byte	5   # 5
+	.byte	159 # DW_OP_stack_value
+	.quad	0
+	.quad	0
 	.section	.debug_abbrev,"",@progbits
 	.byte	1   # Abbreviation Code
 	.byte	17  # DW_TAG_compile_unit
@@ -54,6 +75,8 @@
 	.byte	23  # DW_FORM_sec_offset
 	.byte	64  # DW_AT_frame_base
 	.byte	24  # DW_FORM_exprloc
+	.ascii	"\227B" # DW_AT_GNU_all_call_sites
+	.byte	25  # DW_FORM_flag_present
 	.byte	3   # DW_AT_name
 	.byte	14  # DW_FORM_strp
 	.byte	58  # DW_AT_decl_file
@@ -70,7 +93,7 @@
 	.byte	52  # DW_TAG_variable
 	.byte	0   # DW_CHILDREN_no
 	.byte	2   

[Lldb-commits] [PATCH] D96778: [lldb] Fix handling of `DW_AT_decl_file` according to D91014

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D96778#2565677 , @jankratochvil 
wrote:

> In D96778#2565414 , @werat wrote:
>
>> I can't claim I fully understand what's the difference here, but this aligns 
>> with your comment at https://reviews.llvm.org/D92643#inline-900717 :)
>
> If interested the problem was `DWARFAttributes` can contain attributes 
> collected from multiple DIEs (linked by `DW_AT_specification` or 
> `DW_AT_abstract_origin`). And with (future) DWZ patchset applied for LLDB 
> such DIEs can come from multiple CUs. Therefore it is not enough to assume 
> each attribute comes from CU of the original DIE.
>
> Without DWZ it is sure not a bug. And DWZ is not yet in LLDB.

As @labath mentioned, certainly there's some support in LLDB for cross-CU 
references, as LLVM produces these when performing LTO.

I expect it'd be good to have a test case showing the sort of DWARF that DWZ 
produces for cross-CU references of enumerators.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96778

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

[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

Perhaps all these clone implementations could be provided via mixin instead?

  struct base {
virtual std::shared_ptr Clone() = 0;
...
  };
  template
  struct base_clone_helper : IntermediateBase {
static_assert(std::is_base_of::value);
std::shared_ptr Clone() const override {
  return std::make_shared(static_cast(*this*));
}
  };
  struct some_immediate_derivation final : 
base_clone_helper {
  };
  struct some_intermediate_helper : base {
...
  };
  struct some_concrete_derivation final : 
base_clone_helper {
  };

Requiring that all such types be copy constructible (I guess the template 
helper could also provide an rvalue overload of Clone if there's a need to have 
move cloning, though that seems unlikely to be needed/useful)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96817

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

[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: lldb/unittests/Interpreter/TestOptionValue.cpp:173
+  // Trigger the callback second time.
+  file_list_copy_ptr->SetValueFromString(llvm::StringRef("0 another/path"),
+ eVarSetOperationReplace);

Generally it shouldn't be necessary to write `llvm::StringRef(...)` around a 
string literal - StringRef is implicitly convertible from a string literal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96817

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

[Lldb-commits] [PATCH] D96778: [lldb] Fix handling of `DW_AT_decl_file` according to D91014

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D96778#2567255 , @jankratochvil 
wrote:

> In D96778#2566208 , @dblaikie wrote:
>
>> I expect it'd be good to have a test case showing the sort of DWARF that DWZ 
>> produces for cross-CU references of enumerators.
>
> There is a testcase in D91014 : 
> lldb/test/Shell/SymbolFile/DWARF/DW_AT_decl_file-DW_AT_specification-crosscu.s
> There is no testcase in this patch.

That tests some other cross-cu references, but if it was testing the changes in 
this patch, wouldn't the test be failing (& I guess it isn't)?

So seems like more test coverage might be in order? (Either in another test 
file, or as an addition to the existing one)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96778

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

[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

> CRTP was my first implementation, however, I discarded it as more bug-prone. 
> Virtual Clone function at the interface is so common that, I believe, 
> everyone knows it must be overridden by a new derived class. The necessity of 
> inheriting from base_clone_helper is not so obvious.

I would've thought it'd be pretty easy to accidentally miss either of these - I 
think the CRTP helper ensures consistency of implementation (harder to 
accidentally slice/copy the wrong type/etc. But I'm not a code owner/major 
contributor to lldb specifically, so probably more up to other developers who 
are.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96817

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

[Lldb-commits] [PATCH] D96778: [lldb] Fix handling of `DW_AT_decl_file` according to D91014

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D96778#2568794 , @werat wrote:

> To be honest, I'm not sure how to reproduce this kind of debug info. I've 
> tried a few examples (e.g. force inline the function from another CU) , but 
> it didn't work.
>
> Should we postpone this patch until someone can figure out the test case then?

How'd you come across the issue? Presumably it requires some dwz usage to get, 
but that's OK - if there's a small example using dwz we could probably recreate 
that with some hand crafted assembly without going to great lengcths.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96778

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

[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D96817#2569852 , @clayborg wrote:

> In D96817#2569595 , @dblaikie wrote:
>
>>> CRTP was my first implementation, however, I discarded it as more 
>>> bug-prone. Virtual Clone function at the interface is so common that, I 
>>> believe, everyone knows it must be overridden by a new derived class. The 
>>> necessity of inheriting from base_clone_helper is not so obvious.
>>
>> I would've thought it'd be pretty easy to accidentally miss either of these 
>> - I think the CRTP helper ensures consistency of implementation (harder to 
>> accidentally slice/copy the wrong type/etc. But I'm not a code owner/major 
>> contributor to lldb specifically, so probably more up to other developers 
>> who are.
>
> Whatever is the safest and most llvm like is probably the best approach IMHO. 
> So maybe stick with a solution that will be familiar to LLVM developers if 
> this current approach isn't. I am open to other suggestions if others feel 
> strongly otherwise.

I think either would be legible/familiar to LLVM developers. I rather like the 
CRTP, but it's not a make-or-break.

I found a couple of instances of cloning APIs across LLVM (incomplete results - 
grep only gets one so far):

  clang/include/clang/Sema/TypoCorrection.h:  virtual 
std::unique_ptr clone() = 0;
  lldb/include/lldb/Core/SearchFilter.h:  virtual lldb::SearchFilterSP 
DoCreateCopy() = 0;

And neither uses a CRTP for the derive classes, FWIW.


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

https://reviews.llvm.org/D96817

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

[Lldb-commits] [PATCH] D96778: [lldb] Fix handling of `DW_AT_decl_file` according to D91014

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D96778#2572405 , @jankratochvil 
wrote:

> In D96778#2566208 , @dblaikie wrote:
>
>> I expect it'd be good to have a test case showing the sort of DWARF that DWZ 
>> produces for cross-CU references of enumerators.
>
> Here is a new testcase for the second (`DW_TAG_variable`) fix: 
> https://people.redhat.com/jkratoch/D96778-test.patch
> I cannot update the patch here.

Ah, cool - for the final version of this, I'd suggest the author/whomever rolls 
this into one file with two CUs - bit easier to deal with. You could remove a 
bunch of extraneous DWARF too - since it's hardcoded DWARF, the 
subprogram/inlined subroutine/etc could be removed from CU1, leaving only the 
abstract subprogram and variable, both to be referenced from CU2.

> DWZ can produce such case from GCC output (not used for the testcase): `g++ 
> -o inlinevar inlinevar{1,2}.C -Wall -g -O2;dwz ./inlinevar`
> Such case is never produced clang output as the `DW_TAG_variable declaration` 
> is not merged by `clang -flto` and DWZ for unknown reason does not merge the 
> clang variant. I did merge it for the testcase from clang output by hand.
>
> I did not try a testcase for the enumerators if possible at all.

Could you provide the source code for this - I wouldn't mind trying it out and 
seeing what might be different/why DWZ doesn't understand this.

The LTO point you make is an interesting one too... yeah, for non-inlined (but 
inline linkage) functions LLVM during LTO deduplicates correctly, but this 
doesn't work out once inlining happens.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96778

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

[Lldb-commits] [PATCH] D96778: [lldb] Fix handling of `DW_AT_decl_file` according to D91014

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D96778#2573076 , @jankratochvil 
wrote:

> In D96778#2572816 , @dblaikie wrote:
>
>> rolls this into one file with two CUs - bit easier to deal with.
>
> Then one could not use the `.file` directives and one would need to code also 
> `.debug_line` by hand.

Ah, right, makes sense.

>> Could you provide the source code for this - I wouldn't mind trying it out 
>> and seeing what might be different/why DWZ doesn't understand this.
>
> dwz will merge it without -flto but not with -flto: 
> http://people.redhat.com/jkratoch/inlinevar.d.tar.xz

Ah, yeah, so I see - thanks for the repro!

If I had to guess, it'd be because with LTO clang emits the abstract origin 
lexically after the inlined instance of "var", whereas with non-LTO clang emits 
the abstract origin before the inlined instance.

(& yeah, clang should get this right with lto without the need for dwz - LLVM 
correctly deduplicates types when merging for LTO, but doesn't do so for 
already-inlined functions, could be done with the same sort of logic, though)

> Personally I am not interested in DWZ, I am implementing it only as a 
> compatibility with existing file format, IMNSHO DWZ should be dropped.

Not sure I follow. dwz does use the existing DWARF file format. you mean you're 
only interested in compatibility with the DWARF directly produced by compilers, 
not post-processed by dwz? Fair enough. I guess @werat has some interest in 
supporting dwz for their use cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96778

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

[Lldb-commits] [PATCH] D96778: [lldb] Fix handling of `DW_AT_decl_file` according to D91014

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D96778#2576913 , @jankratochvil 
wrote:

> In D96778#2576881 , @dblaikie wrote:
>
>> In D96778#2573076 , @jankratochvil 
>> wrote:
>>
>>> Personally I am not interested in DWZ, I am implementing it only as a 
>>> compatibility with existing file format, IMNSHO DWZ should be dropped.
>>
>> Not sure I follow. dwz does use the existing DWARF file format. you mean 
>> you're only interested in compatibility with the DWARF directly produced by 
>> compilers, not post-processed by dwz? Fair enough. I guess @werat has some 
>> interest in supporting dwz for their use cases.
>
> No, I need compatibility of LLDB with DWZ (or rather LLDB needs it if it 
> should get used on Red Hat systems).

Ah, OK. You want RH compatibility. RH uses DWZ. You'd rather they didn't, but 
working with it, given they still do use it.

> DWZ is specified by DWARF-5

What part of DWZ is specified by DWARFv5? My understanding is that DWZ uses 
existing standard features/didn't need anything in particular standardized in 
any DWARF version (perhaps the original creation of partial units was motivated 
by DWZ? But partial units have been around for a few versions - seems like it 
was introduced in DWARFv3)

> but there are various DWARF tools still not supporting DWZ.

DWARF is a very broad (as they like to say, "permissive") standard. Lots of 
valid DWARF won't work in lots of places, because the full spectrum of 
interesting ways one could use DWARF features is too broad to be uniformly 
implemented - generally consumers and produces implement enough to work with 
each other and that's about it.

(for instance I recently started dabbling with a feature that would use 
DW_AT_ranges on DW_TAG_subprograms - totally valid DWARF, but unsurprisingly, 
DWARF consumers that'd never had to deal with DWARF like this before didn't 
have support for it (lldb, in particular - had partial support, hopefully it's 
got full support for at least single-length DW_AT_ranges, but I think last I 
checked there were still issues with multi-element ranges))

> Also the only DWZ format currently in use is a non-standard GNU extension of 
> DWARF-4.

Which extensions would those be? At a glance when looking at the small dwz 
example earlier I didn't notice any DWARF extensions. (though, in any case, we 
deal with extensions on a pretty regular basis - see all the DWARFv4 + Split 
DWARF work, for instance (& then the call_site/call_site_parameter stuff, etc))

> The DWZ format has been designed for GDB making it difficult to parse it by 
> LLDB. LLDB does per-DIE DWARF->IR conversion compared to GDB doing per-CU 
> DWARF->IR conversion, that means that LLDB needs to keep context of the DWZ 
> parent DIE (as DWZ recursively imports DW_TAG_partial_unit) for each its 
> reference of DIE in its various indices which is a PITA.

(probably best to leave out the 'PITA'/swear/colourful language)

I'm not sure I follow the description here - when generating Clang ASTs from 
some DIE in LLDB, one would still have to potentially pass through multiple CUs 
to retrieve all the relevant types, especially under any form of LTO - are 
there particular features of DWZ that make this more/differently problematic?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96778

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

[Lldb-commits] [PATCH] D96778: [lldb] Fix handling of `DW_AT_decl_file` according to D91014

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D96778#2576980 , @jankratochvil 
wrote:

> In D96778#2576927 , @dblaikie wrote:
>
>> What part of DWZ is specified by DWARFv5?
>
> Only `DW_*_sup` - being used by DWZ with DWARF-4 as `DW_*_GNU_*_alt` (and 
> dwarfstd.org proposal  
> having it as `DW_*_alt`) for DWZ common files located in 
> `/usr/lib/debug/.dwz/` where each file is for one rpm package sharing DIEs to 
> multiple *.debug files of the same rpm package.

Ah, thanks for the pointers! I didn't know about the cross-file feature of dwz.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96778

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

[Lldb-commits] [PATCH] D98289: [lldb] Fix DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D98289#2618881 , @jankratochvil 
wrote:

> Ah there is already a discussion from it: 
> http://lists.dwarfstd.org/pipermail/dwarf-discuss-dwarfstd.org/2021-March/004765.html

Yeah, happy to discuss/clarify/help with anything in this front either on that 
thread, in this review, or elsewhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98289

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

[Lldb-commits] [PATCH] D98289: [lldb] Fix DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFListTable.h:116
   Optional getOffsetEntry(DataExtractor Data, uint32_t Index) const {
-if (Index > HeaderData.OffsetEntryCount)
+if (Index >= HeaderData.OffsetEntryCount)
   return None;

jankratochvil wrote:
> A regression from: https://reviews.llvm.org/D86110#2621583
Changes to LLVM should have tests in LLVM - probably best to split this out as 
a separate patch to LLVM only, if you can?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98289

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

[Lldb-commits] [PATCH] D86110: [WIP][DebugInfo] Lazily parse debug_loclist offsets

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFListTable.h:116
+  Optional getOffsetEntry(DataExtractor Data, uint32_t Index) const {
+if (Index > HeaderData.OffsetEntryCount)
+  return None;

jankratochvil wrote:
> That is going to be addressed in D98289.
Ah, thanks for the catch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86110

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

[Lldb-commits] [PATCH] D98996: Teach DWARFExpression about DWARF 4+ Location Descriptions

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

(idle question: Is this code remotely related to any code in LLVM's 
libDebugInfoDWARF? Does this patch take us closer to or further away from 
unifying them? If it takes us further away, any chance of designing it in a 
direction that points towards each other rather than away from?)


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

https://reviews.llvm.org/D98996

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

[Lldb-commits] [PATCH] D99120: [lldb] Silence GCC warnings about format not being a string literal in LLDB_SCOPED_TIMER

[David Blaikie via Phabricator via lldb-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: JDevlieghere.

Looks right to me


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99120

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

[Lldb-commits] [PATCH] D97739: Add a progress class that can track and report long running operations that happen in LLDB.

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: lldb/source/API/SBDebugger.cpp:857
+error = m_opaque_sp->GetTargetList().CreateTarget(
+*m_opaque_sp, filename, arch, eLoadDependentsYes, platform_sp,
+target_sp);

jingham wrote:
> clayborg wrote:
> > I submit with "arc diff" and it will cause lint errors if I don't allow it 
> > to fix the lint errors it finds.
> I'm 100% not in favor of tools that force irrelevant changes to be included.  
> But that is a suggested tool so somebody must like that.
They aren't forced - you can submit with linter errors if they don't seem 
helpful.

Pre-committing format changes in standalone NFC commits would generally be 
preferable. & the linter shouldn't be flagging untouched lines - is it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97739

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

[Lldb-commits] [PATCH] D97739: Add a progress class that can track and report long running operations that happen in LLDB.

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: lldb/source/API/SBDebugger.cpp:857
+error = m_opaque_sp->GetTargetList().CreateTarget(
+*m_opaque_sp, filename, arch, eLoadDependentsYes, platform_sp,
+target_sp);

clayborg wrote:
> jingham wrote:
> > dblaikie wrote:
> > > jingham wrote:
> > > > clayborg wrote:
> > > > > I submit with "arc diff" and it will cause lint errors if I don't 
> > > > > allow it to fix the lint errors it finds.
> > > > I'm 100% not in favor of tools that force irrelevant changes to be 
> > > > included.  But that is a suggested tool so somebody must like that.
> > > They aren't forced - you can submit with linter errors if they don't seem 
> > > helpful.
> > > 
> > > Pre-committing format changes in standalone NFC commits would generally 
> > > be preferable. & the linter shouldn't be flagging untouched lines - is it?
> > It looks like the file had changes 700 lines before & 900 lines after, but 
> > these lines weren't changed except by the linter... 
> Actually come to thing about it, it is probably because my editor removes 
> trailing newlines and then the linter deems those lines fair game...
Yeah, if your editor is touching otherwise unmodified lines that'll be a 
problem (for the linter, or without the linter - you'd end up with unrelated 
changes in the diff, etc).

clang-format can be integrated into some editors (vim, for instance) to 
auto-format only changed lines on save, which is what I use - not sure if 
there's an option to use that in your workflow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97739

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

[Lldb-commits] [PATCH] D99401: Fix .debug_aranges parsing issues introduced with llvm error handling in LLDB

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

(generally makes sense to me - but I'll leave it to some more lldb-focussed 
reviewers to do more review/approval)

Usual caveat/question: Does this take us closer or further away from unifying 
this code with LLVM's libDebugInfoDWARF? (or neutral)




Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.cpp:56
+  if (m_header.length > 0)
+m_next_offset = m_offset + *offset_ptr - m_offset + m_header.length;
+  else

This maths seems strange - the `m_offset ... - m_offset` cancel each other out, 
right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99401

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

[Lldb-commits] [PATCH] D99401: Fix .debug_aranges parsing issues introduced with llvm error handling in LLDB

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.cpp:56
+  if (m_header.length > 0)
+m_next_offset = m_offset + *offset_ptr - m_offset + m_header.length;
+  else

clayborg wrote:
> dblaikie wrote:
> > This maths seems strange - the `m_offset ... - m_offset` cancel each other 
> > out, right?
> Math is good. We are adding the number of bytes that were used to encode the 
> length (4 or 12) by saying:
> ```
> m_offset +  + m_header.length
> ```
The net effect ends up a bit convoluted & I think it'd be (at least I'd find 
it) easier to read as `*offset_ptr + m_header.length`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99401

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

[Lldb-commits] [PATCH] D99702: [lldb-vscode] Use LLVM's ScopeExit to ensure we always terminate the debugger

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

@wallace - minor inconvenience, but approving a patch in Phabricator without 
any textual comment does not result in an email to the list, making it look 
like a patch is being committed without approval - could you include some text 
in your Phab approvals, as it makes it a bit easier when doing 
post-commit/process review?

@JDevlieghere Is it worth having a C++ RAII object that calls initialize in the 
ctor and terminate in the dtor? (maybe even removing the init/terminate free 
functions, and making them only accessible through an RAII object if that's 
suitable) What do the other uses of init/terminate look like/could they benefit 
from such refactoring?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99702

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

[Lldb-commits] [PATCH] D98289: [lldb] 2/2: Fix DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s:11
+# Failure was the block range 1..2 was not printed plus:
+# error: DW_AT_range-DW_FORM_sec_offset.s.tmp {0x003f}: DIE has 
DW_AT_ranges(0xc) attribute, but range extraction failed (missing or invalid 
range list table), please file a bug and attach the file at the start of this 
error message
+

I'm not sure if `%t` is guaranteed to produce a file name with that 
format/name. So maybe using `{{.*}}` for the file name (and `{{.*}}-rnglistx` 
below) would be a bit more stable



Comment at: lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s:32
+# RNGLISTBASE-LABEL: image lookup -v -s lookup_rnglists
+# RNGLISTBASE: error: DW_AT_range-DW_FORM_sec_offset.s.tmp-rnglistbase 
{0x0043}: DIE has DW_AT_ranges(0x0) attribute, but range extraction failed 
(invalid range list table index 0), please file a bug and attach the file at 
the start of this error message
+

Perhaps this could be more informative about what makes the range list index of 
0 invalid? "index 0 out of range of range list table (with range list base 
0xXXX) with offset entry count of XX (valid indexes 0-(XX-1))" Maybe that's too 
verbose/not worth worrying about since this'll only be relevant to DWARF 
producers trying to debug their DWARFv5, maybe no one will ever see this 
message in practice. Just a thought.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98289

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

[Lldb-commits] [PATCH] D100447: [lldb] Silence GCC warnings about control reaching the end of non-void functions. NFC.

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

I'm guessing this change will break the build in a different way, triggering 
clang's -Wcovered-switch-default - the usual way this gcc/clang diagnostic 
pinch is resolved is by putting the unreachable after the switch/at the end of 
the function, instead of in a default case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100447

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

[Lldb-commits] [PATCH] D100447: [lldb] Silence GCC warnings about control reaching the end of non-void functions. NFC.

[David Blaikie via Phabricator via lldb-commits]

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

Looks good - thanks! Do you need me to commit this for you?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100447

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

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D100191#2689543 , @mgorny wrote:

> In D100191#2689489 , @labath wrote:
>
>> We should also start thinking about tests. I suppose the smallest piece of 
>> functionality that could be usefully tested (with a lldb-server test) is 
>> debugging a process that forks, stopping after the fork, and detaching from 
>> the child. Shall we try making that work first?
>
> I'm really too exhausted on this to write more tests. A few commits later I'm 
> adding a full set of basic tests for various fork scenarios. I think the 
> patches are simple enough to justify limiting ourselves to integration 
> testing for the whole thing, at least for the time being.

Generally LLVM patches must have accompanying tests for any code they're adding 
- if it's not possible to test the patch because it's too isolated/lacks 
related infrastructure then it's possible the patch is too small. But we try 
pretty hard to isolate patches and find ways to test work incrementally as it's 
added.


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

https://reviews.llvm.org/D100191

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

[Lldb-commits] [PATCH] D100806: [lldb] Fix one leak in reproducer

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: lldb/tools/driver/Driver.cpp:856
   if (const char *reproducer_path = SBReproducer::GetPath()) {
-// Leaking the string on purpose.
-std::string *finalize_cmd = new std::string(argv0);
+static std::string *finalize_cmd = new std::string(argv0);
 finalize_cmd->append(" --reproducer-finalize '");

Can this code execute more than once? If it does I guess it'll really leak?

(if it's not meant to be called more than once, maybe something like:
```
static std::string *x = nullptr;
assert(!x);
x = new std::string(argv0);
...
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100806

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

[Lldb-commits] [PATCH] D100806: [lldb] Fix one leak in reproducer

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: lldb/tools/driver/Driver.cpp:856
   if (const char *reproducer_path = SBReproducer::GetPath()) {
-// Leaking the string on purpose.
-std::string *finalize_cmd = new std::string(argv0);
+static std::string *finalize_cmd = new std::string(argv0);
 finalize_cmd->append(" --reproducer-finalize '");

MaskRay wrote:
> dblaikie wrote:
> > Can this code execute more than once? If it does I guess it'll really leak?
> > 
> > (if it's not meant to be called more than once, maybe something like:
> > ```
> > static std::string *x = nullptr;
> > assert(!x);
> > x = new std::string(argv0);
> > ...
> > ```
> The function is only executed once. 
If that's not enforced in any way (at least I can't see any) & the rest of the 
code looks like it would be OK being called more than once (I think?) - might 
be worth some checking?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100806

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

[Lldb-commits] [PATCH] D137838: [Support] Move TargetParsers to new component

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

This has introduced a circular dependency due to the forwarding header 
(forwarding header depends on new lib, new lib depends on support, where the 
forwarding header is). Generally this wouldn't be acceptable (& I'd suggest the 
patch be reverted on that basis) though I understand this is a complicated 
migration - what's the timeline for correcting this issue?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137838

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

[Lldb-commits] [PATCH] D139379: [llvm][dwwarf] Change CU/TU index to 64-bit

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D139379#3972876 , @ayermolo wrote:

> In D139379#3972871 , @dblaikie 
> wrote:
>
>> Perhaps the change to use accessors could be removed, now that you've used 
>> it to find all the places that needed to be fixed up? (like just using it 
>> for cleanup/temporary purposes, without needing to commit that API change?)
>
> I am not sure what other projects are using it, that I missed, or not in 
> llvm-trunk, but are based of it.

It's awkward to convolute the API to ensure a breakage - I think it's best to 
leave it as-is, for the most part. I guess you needed the 32 bit accessors so 
existing code doesn't become UB because of truncation to 32 bit?

Could the code keep the existing member names, provide the wrappers without 
turning the members into an array and needing named index constants, etc, at 
least? (though even then, seems like there's more to it than needed)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139379

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

[Lldb-commits] [PATCH] D139379: [llvm][dwwarf] Change CU/TU index to 64-bit

[David Blaikie via Phabricator via lldb-commits]

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



In D139379#4015624 , @ayermolo wrote:

> In D139379#4015569 , @dblaikie 
> wrote:
>
>> In D139379#3972876 , @ayermolo 
>> wrote:
>>
>>> In D139379#3972871 , @dblaikie 
>>> wrote:
>>>
 Perhaps the change to use accessors could be removed, now that you've used 
 it to find all the places that needed to be fixed up? (like just using it 
 for cleanup/temporary purposes, without needing to commit that API change?)
>>>
>>> I am not sure what other projects are using it, that I missed, or not in 
>>> llvm-trunk, but are based of it.
>>
>> It's awkward to convolute the API to ensure a breakage - I think it's best 
>> to leave it as-is, for the most part. I guess you needed the 32 bit 
>> accessors so existing code doesn't become UB because of truncation to 32 bit?
>>
>> Could the code keep the existing member names, provide the wrappers without 
>> turning the members into an array and needing named index constants, etc, at 
>> least? (though even then, seems like there's more to it than needed)
>
> Thanks for circling back to this during holidays. :)
> Right, that was my original thought pattern. I am fully open to the idea that 
> this is not the right approach. :)

I don't know that there's enough out of tree usage to worry about forcing a 
break by changing the API like this - or to include the "get*32" functions.

Is this actually an undefined behavior issue (I don't think the implicit 
conversion would be any more broken than the explicit cast, at least - but 
can't find the specific wording about defined/undefined behavior off hand at 
the moment) , or only an attempt to identify places that could benefit from 
updates to support 64 bit lengths?

Eh, still seems weird, but guess it's not that much of a big deal - so let's go 
with it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139379

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

[Lldb-commits] [PATCH] D142672: [lldb] Make SBSection::GetSectionData call Section::GetSectionData.

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: lldb/source/API/SBSection.cpp:187-189
+DataExtractorSP result_data_sp =
+std::make_shared(section_data, offset, size);
+sb_data.SetOpaque(result_data_sp);

Probably either use `std::move` when passing `result_data_sp` to `SetOpaque`, 
or roll the expressions together (to avoid an unnecessary copy of a ref counted 
smart pointer, that would cause extra increment/decrement of the ref count)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142672

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

[Lldb-commits] [PATCH] D143501: [clang][DebugInfo] lldb: Use preferred name's type when emitting DW_AT_names

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D143501#4110302 , @Michael137 
wrote:

> The alternative would be attaching the preferred name as a new attribute or 
> an existing attribute like `DW_AT_description`.

I'd recommend a possible long-term solution would be simplified template names 
(so we don't have to worry about encoding this in the `DW_AT_name` anyway) and 
a "DW_AT_LLVM_preferred_name" or similar attribute on a type that refers to the 
typedef that is the preferred name. This would generalize further than only 
appearing in template names - the type of a variable inside a template would 
also gain the beneficial naming (eg: `template void f1(T t) { }` - 
as it stands, the type of the variable `t` must be `std::basic_stringhttps://reviews.llvm.org/D143501/new/

https://reviews.llvm.org/D143501

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

[Lldb-commits] [PATCH] D143501: [clang][DebugInfo] lldb: Use preferred name's type when emitting DW_AT_names

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a subscriber: probinson.
dblaikie added a comment.

In D143501#4116200 , @Michael137 
wrote:

>> I'd recommend a possible long-term solution would be simplified template 
>> names (so we don't have to worry about encoding this in the `DW_AT_name` 
>> anyway) and a "DW_AT_LLVM_preferred_name" or similar attribute on a type 
>> that refers to the typedef that is the preferred name. This would generalize 
>> further than only appearing in template names - the type of a variable 
>> inside a template would also gain the beneficial naming (eg: 
>> `template void f1(T t) { }` - as it stands, the type of the 
>> variable `t` must be `std::basic_string> `DW_TAG_class_type` for `std::basic_string> attribute on it, then a debugger could helpfully render the type by its 
>> preferred alias instead)
>
> That could be a neat solution. I probably asked this before, but what's the 
> timeline with switching it on by default (if such plan is in the works at 
> all)?

I haven't especially planned that - though given it's been picked up by Fuschia 
and Chromium (at least in some build modes), and we got gdb and lldb mostly 
fixed, maybe it's worth considering. Defaulting on for lldb might be easier 
than for gdb (gdb has some outstanding index bugs with it). But generally I 
figure you Apple folks tend to be the ones who have more investment in what the 
lldb tuning should cover, or not?

If you folks want to try turning it on & we could see about turning it on by 
default for lldb tuning?

>> Alternatively, I suppose, the DWARF emission could just look at the 
>> preferred name and use that as the `DW_AT_type` in all cases anyway? Avoids 
>> needing a new attribute, etc, though would be a bit quirky in its own way.
>
> This is how I first thought of implementing it, but ran into some circular 
> dependency quirks and started working on this patch instead. Could take a 
> second stab at doing so if that's the preference. Would be nice to not have 
> this behind a tuning

yeah, happy to help with pointers, etc.

I think /maybe/ we had some design principle that DWARF features wouldn't be 
solely controlled by debugger tuning, the tuning only indicates defaults but 
tehy can be controlled by flags? Not sure if I'm remembering that quite right, 
though - maybe @probinson remembers more of that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143501

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

[Lldb-commits] [PATCH] D143501: [clang][DebugInfo] lldb: Use preferred name's type when emitting DW_AT_names

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

> I think /maybe/ we had some design principle that DWARF features wouldn't be 
> solely controlled by debugger tuning, the tuning only indicates defaults but 
> tehy can be controlled by flags? Not sure if I'm remembering that quite 
> right, though - maybe @probinson remembers more of that?

I guess maybe not - at least there's a handful of debugger tuning checks in 
similar code around here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143501

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

[Lldb-commits] [PATCH] D145609: [lldb] Change dwim-print to default to disabled persistent results

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

> The rationale is: dwim-print doesn't always use expression evaluation, it 
> prefers to use frame variable where possible. In the future it could be 
> expanded, for example to print register as well. Because dwim-print doesn't 
> always use expression, there isn't always a persistent result.

Oh, so in the cases where `dwim-print` delegates to `frame variable` you can't 
then refer to that result in another `dwim-print` command/other place where 
you'd like to reference a previously printed value?

That seems like a significant regression over existing print functionality and 
compared to gdb. At least for me that'd be annoying/inconvenient to have to 
think about which kind of printing I'm doing (& have to explicitly use the less 
stable full expression based printing) when I want to reuse a value.

> To make dwim-print output consistent, and because it's presumed most users 
> don't use persistent results

Got data on that? I'd /guess/ I use it maybe in single digit % of my prints, 
but probably at least 1%, though no idea if that's representative - even if it 
is, the inconsistency feels counter to the goals of dwim-print & the 
convenience provided by gdb's consistent value handling.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145609

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

[Lldb-commits] [PATCH] D145803: [clang][DebugInfo] Emit DW_AT_type of preferred name if available

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: clang/test/CodeGen/preferred_name.cpp:49
+
+Foo> varFooInt;
+

Michael137 wrote:
> probinson wrote:
> > This doesn't become `Foo` ?
> The name stays as `Foo>` but the type of the template parameter 
> becomes `BarInt`. So the dwarf would look like:
> ```
> DW_TAG_structure_type
>   DW_AT_name  ("Foo >")
> 
>   DW_TAG_template_type_parameter
> DW_AT_type(0x0095 "BarInt")
> DW_AT_name("T")
> 
> ```
> Will add the parameter metadata to the test
Hmm, that seems buggy - why doesn't `Foo >` become `Foo`? (is 
the preferred name ever used in the DW_AT_name? I'd have thought it should be 
unless it's explicitly avoided to ensure type compatibility/collapsing with 
debug info built without this feature enabled?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145803

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

[Lldb-commits] [PATCH] D145803: [clang][DebugInfo] Emit DW_AT_type of preferred name if available

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: clang/test/CodeGen/preferred_name.cpp:49
+
+Foo> varFooInt;
+

Michael137 wrote:
> dblaikie wrote:
> > Michael137 wrote:
> > > probinson wrote:
> > > > This doesn't become `Foo` ?
> > > The name stays as `Foo>` but the type of the template parameter 
> > > becomes `BarInt`. So the dwarf would look like:
> > > ```
> > > DW_TAG_structure_type
> > >   DW_AT_name  ("Foo >")
> > > 
> > >   DW_TAG_template_type_parameter
> > > DW_AT_type(0x0095 "BarInt")
> > > DW_AT_name("T")
> > > 
> > > ```
> > > Will add the parameter metadata to the test
> > Hmm, that seems buggy - why doesn't `Foo >` become `Foo`? 
> > (is the preferred name ever used in the DW_AT_name? I'd have thought it 
> > should be unless it's explicitly avoided to ensure type 
> > compatibility/collapsing with debug info built without this feature 
> > enabled?)
> I suspect it's because the name is constructed using the clang TypePrinter. 
> And we turn off `PrintingPolicy::UsePreferredNames` by default to avoid 
> divergence from GCC.
> 
> Will double check though
Yeah, that sounds right/OK to me. Sorry for the diversion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145803

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

[Lldb-commits] [PATCH] D147292: [lldb] Add support for the DW_AT_trampoline attribute with a boolean

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D147292#4238834 , @augusto2112 
wrote:

> In D147292#4238820 , @JDevlieghere 
> wrote:
>
>> There seems to be overlap in the code added in this patch and D146679 
>> . Does one supersede the other or is there 
>> a dependency? If it's the latter you should extract that into a separate 
>> patch and make it a parent revision of this and D146679 
>> .
>
> Yes, sorry, I'm abandoning the other one in favor of this one.

Be handy to mark this abandoned when you get a chance


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147292

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

[Lldb-commits] [PATCH] D106466: [llvm+lldb] Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

I assume there's already test coverage for rnglistx in debug_info.dwo/split 
unit? (because in that case there's no rnglists_base, but rnglistx is usable) - 
could you explain how this code avoids treating the split unit rnglists_base == 
0 case as "there is no rnglists_base and so rnglistx isn't usable"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106466

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

[Lldb-commits] [PATCH] D106466: [llvm+lldb] Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)

[David Blaikie via Phabricator via lldb-commits]

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

In D106466#2950268 , @jankratochvil 
wrote:

> In D106466#2947710 , @dblaikie 
> wrote:
>
>> I assume there's already test coverage for rnglistx in debug_info.dwo/split 
>> unit? (because in that case there's no rnglists_base, but rnglistx is usable)
>
> I admit I did not check it, thanks for catching it. But it is already tested 
> by lldb/test/Shell/SymbolFile/DWARF/x86/debug_rnglists-dwo.s 
> 
>  implemented by Pavel Labath in 2019 
> .
>  It is using DWO `DW_TAG_lexical_block->DW_AT_ranges->DW_FORM_rnglistx`.
>
>> - could you explain how this code avoids treating the split unit 
>> rnglists_base == 0 case as "there is no rnglists_base and so rnglistx isn't 
>> usable"?
>
> `m_ranges_base` is not zero in such case as it has been set from the skeleton 
> .

ah, OK - was going to say that setting it from the skeleton would be incorrect 
(for DWARF 5, at least) - but I see that's handled a few lines down: 
https://github.com/llvm/llvm-project/blob/f58a642da19c64cb6ee1badb0f176a872c1d7a0a/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp#L110

Looks all good to me, then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106466

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

[Lldb-commits] [PATCH] D108228: Fix error handling in the libcxx std::string formatter

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/main.cpp:101
 S.assign(L"!"); // Set break point at this line.
+std::string *not_a_string = (std::string *) 0x0;
+touch_string(*not_a_string);

This cast isn't needed, right? This could be rewritten more traditionally as:
```
std::string ¬_a_string = nullptr;
```
?



Comment at: 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/main.cpp:102
+std::string *not_a_string = (std::string *) 0x0;
+touch_string(*not_a_string);
 return 0;

JDevlieghere wrote:
> shafik wrote:
> > This is undefined behavior and I AFAICT this won't pass a sanitized build, 
> > amazingly even if I use `__attribute__((no_sanitize("address", 
> > "undefined")))` : https://godbolt.org/z/4TGPbrYhq
> Definitely UB, but the sanitized bot builds LLDB with the sanitizers, not the 
> test cases, so this should be "fine". 
Seems best avoided if possible though, yeah? What's trying to be demonstrated 
by this test?

What if the function took a std::string* instead of std::string&, and the 
caller doesn't need to dereference that pointer - it could call some other, 
unrelated function to act as a stop-point for the debugger?

& then the "printing a bad string" Could be tested by printing an expression, 
like "p *str" that dereferences in the expression?

Or is the issue only present through the auto-printing of variables in 
parameters in a stack trace, and not present when using the user-defined 
expression evaluator?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108228

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

[Lldb-commits] [PATCH] D108228: Fix error handling in the libcxx std::string formatter

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/main.cpp:102
+std::string *not_a_string = (std::string *) 0x0;
+touch_string(*not_a_string);
 return 0;

teemperor wrote:
> dblaikie wrote:
> > JDevlieghere wrote:
> > > shafik wrote:
> > > > This is undefined behavior and I AFAICT this won't pass a sanitized 
> > > > build, amazingly even if I use `__attribute__((no_sanitize("address", 
> > > > "undefined")))` : https://godbolt.org/z/4TGPbrYhq
> > > Definitely UB, but the sanitized bot builds LLDB with the sanitizers, not 
> > > the test cases, so this should be "fine". 
> > Seems best avoided if possible though, yeah? What's trying to be 
> > demonstrated by this test?
> > 
> > What if the function took a std::string* instead of std::string&, and the 
> > caller doesn't need to dereference that pointer - it could call some other, 
> > unrelated function to act as a stop-point for the debugger?
> > 
> > & then the "printing a bad string" Could be tested by printing an 
> > expression, like "p *str" that dereferences in the expression?
> > 
> > Or is the issue only present through the auto-printing of variables in 
> > parameters in a stack trace, and not present when using the user-defined 
> > expression evaluator?
> > This is undefined behavior and I AFAICT this won't pass a sanitized build, 
> > amazingly even if I use __attribute__((no_sanitize("address", 
> > "undefined"))) : https://godbolt.org/z/4TGPbrYhq
> 
> This is just a segfault unrelated to sanitizers.
> 
> > & then the "printing a bad string" Could be tested by printing an 
> > expression, like "p *str" that dereferences in the expression?
> > Or is the issue only present through the auto-printing of variables in 
> > parameters in a stack trace, and not present when using the user-defined 
> > expression evaluator?
> 
> The expression evaluator is already erroring out when printing that variable 
> (or doing the `*str` equivalent) so I believe this is just about directly 
> accessing the variables. The same goes for situations like `frame var *str` 
> which already handle the error that we now also check for here. I *believe* 
> we really need a the null reference as in the current test so that we end up 
> in a situation where the formatter has to handle the "parent is null" error 
> itself in this code path because of a null reference.
Ah, that's a pity. Oh well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108228

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

[Lldb-commits] [PATCH] D97786: LLDB: Use path relative to binary, not relative to debugger CWD, for finding .dwo files.

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D97786#2972153 , @pfaffe wrote:

> This change breaks all existing uses of relative comp_dirs that don't 
> accidentally make all of them relative to the executable's directory already.
>
> It's easy to construct broken use cases: Consider compiling your program like 
> `clang -g ./src/src.c -gsplit-dwarf -o ./out/src 
> -fdebug-prefix-map=$(pwd)=.`. This will create `src.dwo` in $(pwd) and set 
> the DW_AT_comp_dir of `src` to ".";  the change makes it impossible to load 
> `src.dwo`.
>
> The problem is easy to see in this slightly constructed example and could be 
> fixed here by moving around the dwo files or setting the prefix to '..' to 
> make it point to the executable. But that's not always possible! What if we 
> have a static library that gets linked into multiple executables in different 
> locations? What do we set comp_dir to in the library objects?
>
> The change also introduces different behavior for loading dwos vs. source 
> files. The latter will still be loaded relative to the CWD.

This doesn't solve all use cases/it's not a terribly general purpose situation 
- using -fdebug-prefix-map/-fdebug-compilation-dir requires knowledge of these 
limitations/how to use these features together. (though I agree that changes in 
this area should apply to all relative comp_dir lookups - source and dwo files 
alike, rather than handling one group differently from the other when they're 
all defined as comp_dir relative)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97786

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

[Lldb-commits] [PATCH] D97786: LLDB: Use path relative to binary, not relative to debugger CWD, for finding .dwo files.

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D97786#2974202 , @pfaffe wrote:

> In D97786#2973868 , @dblaikie wrote:
>
>> This doesn't solve all use cases/it's not a terribly general purpose 
>> situation - using -fdebug-prefix-map/-fdebug-compilation-dir requires 
>> knowledge of these limitations/how to use these features together. (though I 
>> agree that changes in this area should apply to all relative comp_dir 
>> lookups - source and dwo files alike, rather than handling one group 
>> differently from the other when they're all defined as comp_dir relative)
>
> My main concern is that even with knowledge of how the debug-compilation-dir 
> features work, there are use cases that stop working after this change and 
> are unfixable on the user's end. Anything that involves linking shared 
> objects into multiple executables for example.

Not fixable? Not sure I follow. In the case of cwd-relative (old behavior) you 
can fix the behavior by changing where you invoke the debugger from, and in the 
case of exe-relative you can fix the behavior by moving the executable? (once 
both source and dwo lookup is implemented consistently, at least)

Do you know anyone using the feature that would be challenged by this change in 
behavior? The only place I know using the feature is Chromium. (Google's 
internal build system uses -fdebug-compilation-dir=/proc/cwd so it really gets 
cwd-relative always, even with this change in lldb (but it's not a portable 
solution, since the path is unix-ish-specific, whereas Chromium probably needs 
a more portable solution that works on Windows, etc) - and then some local 
patches to gdb I think to do change the search paths so comp_dir gets ignored 
anyway, I think? not 100% sure, it's all a bit arcane)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97786

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

[Lldb-commits] [PATCH] D97786: LLDB: Use path relative to binary, not relative to debugger CWD, for finding .dwo files.

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D97786#2976549 , @pfaffe wrote:

> In D97786#2974879 , @dblaikie wrote:
>
>> Not fixable? Not sure I follow. In the case of cwd-relative (old behavior) 
>> you can fix the behavior by changing where you invoke the debugger from, and 
>> in the case of exe-relative you can fix the behavior by moving the 
>> executable? (once both source and dwo lookup is implemented consistently, at 
>> least)
>
> Thanks, I totally missed the fact that the patch intended to keep 
> cwd-relative lookup working!

Hmm, sorry, I didn't mean to imply that/didn't know the patch did that (perhaps 
it only does by accident, I'm not sure).

What I meant was - no matter which interpretation (binary relative or cwd 
relative) is provided by a DWARF consumer, there's a way to make it work 
(assuming the paths in the DWARF/across all the CUs are consistently relative 
to the same thing) - either you move the binary, or you move where you invoke 
the binary from/your cwd. So I don't think either interpretation makes 
something unworkable/unusable, depending on where you define the constraints of 
the problem (if you define it as "there's something you can do when 
compiling/linking the binary to get a certain behavior to work" (ie: you can 
always get cwd-relative if that's what you want), then yes, there's no solution 
if the change is made to make it binary-relative - but if you define the 
problem as "there's a way to debug this binary given these relative paths" - 
then yes, I think there's an answer either way: move the binary, or move the 
cwd). Then it's a question of backwards compatibility (I don't know of anyone 
relying on the cwd-relative behavior, do you? the only place using this 
functionality that I know of is Chromium) and a general "which is the better 
behavior", which, again, if it's basically only Chromium using it/built for 
that situation, I think it's fair to say they probably get to say which is more 
suitable for their use case.

(all this said: Chromium already had a debugger configuration (at least for 
gdb?) to set the `directories` property ( 
https://sourceware.org/gdb/onlinedocs/gdb/Source-Path.html ) - which basically 
overrides/implements the comp_dir relative searching (by initializing the 
`directories` property, by default, with `$cdir;$cwd` (so it searches a given 
DWARF-specified-comp_dir-relative-path relative to DW_AT_comp_dir, and if that 
fails, searches relative to $cwd) - which meant that it really didn't matter 
what DW_AT_comp_dir they used, it could've been complete garbage and 
wouldn't've mattered) - but that hadn't extended to dwo searching, so they had 
a problem with dwo searching, which, imho, the right solution would've been to 
extend gdb's use of its `directories` property to be used also for dwo 
searching, since it's the mechanism for comp_dir relative path resolution and 
dwo paths are comp_dir relative - then there wouldn't've been much need to 
change anything, so far as I know... (and some similar functionality for lldb 
would be nice too))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97786

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

[Lldb-commits] [PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D109345#2986297 , @thopre wrote:

> Is there no way to split this patch further? It's going to be hard finding 
> someone who can review something so big. If there's no way to split it in 
> incremental changes, you could perhaps split per subsystem only for review 
> and refer to this diff for CI as well as when landing.

The long migration path would be to do this one function at. time (I did a 
whole cluster of functions in MemoryBuffer for consistency - this does reduce 
total code changes somewhat, since some of those APIs are used in similar 
contexts (eg: branches of a conditional operator - so having them differ means 
more revisions to those call sites)) and probably introducing separate names 
for the Expected versions of the functions, migrating call sites incrementally, 
then doing a mechanical rename at the end of all that.

I don't think it's probably worth that level of granularity - it's a fairly 
mechanical patch as it is.

Mostly I sent this out as an FYI and to get feedback on the general direction - 
whether folks thought it was worth doing at all, and if they feel strongly it 
should be done another way (like the incremental ones above) - but I don't 
think it needs a /huge/ amount of scrutiny, review by separate code owners, 
etc. I'd generally be comfortable committing this as other cross-project 
cleanup/refactoring like function renaming, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109345

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

[Lldb-commits] [PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D109345#2987565 , @dexonsmith 
wrote:

> This seems like the right direction to me! Especially like the 
> look-through-the-ErrorInfo change for `FileError` -- I hit this before and 
> found it annoying.

Thanks for taking a look!

> IMO, it'd be valuable to split out the consequential functional changes:
>
> - Improvements/changes you made to FileError could be committed ahead of time

Sure sure, can be committed and unit tested separately for sure.

> - Other improvements you discussed to avoid regressions in (e.g.) 
> llvm-symbolizer (seems potentially important?)

I didn't think the yaml symbolizer output was that important - but turned out 
not to be super hard to fix, so I've done that. (were there other regressions I 
mentioned/should think about?)

> I agree the other changes are mostly mechanical and don't all need careful 
> review-by-subcomponents.
>
> That said, it looks very painful for downstream clients of the LLVM C++ API 
> since it's structured as an all-or-nothing change.

Yeah, certainly crossed my mind.

> Especially for managing cherry-picks to long-lived stable branches. It's 
> painful because clients will have code like this:
>
>   if (auto MaybeFile = MemoryBuffer::getFileOrSTDIN()) {
> // Do something with MaybeFile
>   }
>   // Else path doesn't care about the specific error code.
>
> that will suddenly start crashing at runtime. I even wonder if there like 
> that introduced in-tree by your current all-in-one patch, since I think our 
> filesystem-error paths are often missing test coverage. (It'd be difficult to 
> do a thorough audit.)

Yeah, that came up in a handful of cases that used 'auto' (without using auto 
it's a compile failure).

> I thought through a potential staging that could mitigate the pain:
>
> 1. Add `using MemoryBufferCompat = MemoryBuffer` and search/replace all 
> static `MemoryBuffer::` API calls over to `MemoryBufferCompat::`. No direct 
> impact on downstreams.

Yeah, that's some of the extra churn I was thinking of/hoping to avoid - but 
yeah, it's probably worthwhile.

What's the benefit of having the extra step where everything's renamed twice? 
(if it's going to be a monolithic commit - as mentioned in (3)) Compared to 
doing the mass change while keeping the (1 & 2) pieces for backwards 
compatibility if needed?

> 2. Change `MemoryBuffer` to use `Error` and `Expected`, leaving behind 
> `std::error_code` and `ErrorOr` wrappers in a no-longer-just-a-typedef 
> `MemoryBufferCompat`. Easy for downstreams to temporarily revert, and 
> cherry-pick once they've finished adapting in the example of (1).
> 3. Update all code to use the new APIs. Could be done all at once since it's 
> mostly mechanical, as you said. Also an option to split up by component 
> (e.g., maybe the llvm-symbolizer regression is okay, but it could be nice to 
> get that reviewed separately / in isolation).
> 4. Delete `MemoryBufferCompat`. Downstreams can temporarily revert while they 
> follow the example of on (3).
>
> (Given that (1) and (2) are easy to write, you already have `tree` state for 
> (4), and (3) is easy to create from (4), then I //think// you could construct 
> the above commit sequence without having to redo all the work... then if you 
> wanted to split (3) up from there it'd be easy enough.)
>
> (2) seems like the commit mostly likely to cause functional regressions, and 
> it'd be isolated and easy to revert/reapply (downstream and/or upstream) 
> without much churn.

(3) would be more likely to cause regression? Presumably (2) is really 
uninteresting because it adds a new API (re-adding MemoryBuffer, but unused 
since everything's using MemoryBufferCompat) without any usage, yeah?

Plausible, though a fair bit more churn - I'd probably be up for it, though.

- Dave


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109345

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

[Lldb-commits] [PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D109345#2990577 , @dexonsmith 
wrote:

> In D109345#2990527 , @dblaikie 
> wrote:
>
>> (were there other regressions I mentioned/should think about?)
>
> I don't have specific concerns; I was just reading between the lines of your 
> description...

Fair - probably my own hedging there.

>>> 1. Add `using MemoryBufferCompat = MemoryBuffer` and search/replace all 
>>> static `MemoryBuffer::` API calls over to `MemoryBufferCompat::`. No direct 
>>> impact on downstreams.
>>
>> Yeah, that's some of the extra churn I was thinking of/hoping to avoid - but 
>> yeah, it's probably worthwhile.
>>
>> What's the benefit of having the extra step where everything's renamed 
>> twice? (if it's going to be a monolithic commit - as mentioned in (3)) 
>> Compared to doing the mass change while keeping the (1 & 2) pieces for 
>> backwards compatibility if needed?
>
> Benefits I was seeing of splitting (1-3) up are:
>
> - makes (2) easy for downstreams to integrate separately from (1) and (3) 
> (see below for why (2) is interesting)
> - prevents any reverts of (3) on main from resulting in churn in downstream 
> efforts to migrate in response to (1-2)
> - enables (3) to NOT be monolithic -- still debatable how useful it is, but 
> if split up then individual pieces can run through buildbots separately (and 
> be reverted separately)
>
>>> 2. Change `MemoryBuffer` to use `Error` and `Expected`, leaving behind 
>>> `std::error_code` and `ErrorOr` wrappers in a no-longer-just-a-typedef 
>>> `MemoryBufferCompat`. Easy for downstreams to temporarily revert, and 
>>> cherry-pick once they've finished adapting in the example of (1).
>>> 3. Update all code to use the new APIs. Could be done all at once since 
>>> it's mostly mechanical, as you said. Also an option to split up by 
>>> component (e.g., maybe the llvm-symbolizer regression is okay, but it could 
>>> be nice to get that reviewed separately / in isolation).
>>> 4. Delete `MemoryBufferCompat`. Downstreams can temporarily revert while 
>>> they follow the example of on (3).
>>>
>>> (Given that (1) and (2) are easy to write, you already have `tree` state 
>>> for (4), and (3) is easy to create from (4), then I //think// you could 
>>> construct the above commit sequence without having to redo all the work... 
>>> then if you wanted to split (3) up from there it'd be easy enough.)
>>>
>>> (2) seems like the commit mostly likely to cause functional regressions, 
>>> and it'd be isolated and easy to revert/reapply (downstream and/or 
>>> upstream) without much churn.
>>
>> (3) would be more likely to cause regression? Presumably (2) is really 
>> uninteresting because it adds a new API (re-adding MemoryBuffer, but unused 
>> since everything's using MemoryBufferCompat) without any usage, yeah?
>
> (2) changes all downstream clients of MemoryBuffer APIs from 
> `std::error_code` to `Error`
>
> - Mostly will cause build failures
> - Also runtime regressions, due to `auto`, etc.

Oooh, right. Good point - thanks for walking me through it!

> The fix is to do a search/replace of `MemoryBuffer::` to 
> `MemoryBufferCompat::` on only the downstream code.
>
> - Splitting from (1) means you can sequence this change between (1) and (2) — 
> code always builds.
> - Splitting from (3) means you can do a simple search/replace. If (2) is 
> packed up with (3) it seems a lot more awkward, since you have to avoid 
> accidentally undoing (3) during the search/replace. Then if somehow (3) gets 
> reverted you need to start over when it relands.

Yeah, think I'm with you.

Given the amount of churn this means, though - reckon it's worth it? Reckon it 
needs more llvm-dev thread/buy-in/etc? Any other bike-shedding for 
MemoryBufferCompat? (MemoryBufferDeprecated? but I don't really mind)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109345

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

[Lldb-commits] [PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

Thanks for the suggestions/details, @dexonsmith  - I've posted to llvm-dev 
here: https://groups.google.com/g/llvm-dev/c/m9UVRhzJvh4/m/qdd_SyPuCQAJ and 
will wait for some follow-up (or dead silence) before starting along probably 
your latter suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109345

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

[Lldb-commits] [PATCH] D110571: [lldb] Add omitted abstract formal parameters in DWARF symbol files

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

>> - We could supply a test written in C, but it needs -O1 and is fairly 
>> sensitive to the meaning of -O1 (e.g., clang started inlining and omitting 
>> unsued inlined parameters only recently, so changes to -O1 could make a C 
>> test easily meaningless). Any concerns here?
>
> It is really hard to make sure the compiler generates what you want for a 
> test case as it will change over time and you might not end up testing what 
> you think you are testing. The easiest way to avoid this is to emit the 
> assembly from the compiler and then use that as the source for the test since 
> that will guarantee the same output.

If anyone ever needs a hand constructing a stable debug info test case using 
clang (or other compilers for that matter) - I'm totally happy to help. It's 
quite possible to constrain the compiler enough and give it easy enough things 
to inline to make it pretty reliable - for instance for this sort of issue, I'd 
expect something like this is what I'd use to demonstrate a missing parameter:

  __attribute__((optnone)) __attribute__((nodebug)) void use(int*) { }
  inline void f1(int a, int b) {
use(&b);
  }
  int main() {
f1(5,  6);
  }

This compiled with some optimizations (-O1 or above should be adequate) should 
result in a single concrete subprogram for main, a single inlined subroutine 
with a single formal parameter in the inlined subroutine (for 'b') and the 
abstract origin will have both 'a' and 'b'.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110571

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

[Lldb-commits] [PATCH] D110455: DebugInfo: Use clang's preferred names for integer types

[David Blaikie via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf6a561c4d675: DebugInfo: Use clang's preferred names 
for integer types (authored by dblaikie).
Herald added subscribers: llvm-commits, lldb-commits, ormris, hiraditya.
Herald added projects: LLDB, LLVM.

Changed prior to commit:
  https://reviews.llvm.org/D110455?vs=374985&id=377716#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110455

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGen/aarch64-debug-sve-vector-types.c
  clang/test/CodeGen/aarch64-debug-sve-vectorx2-types.c
  clang/test/CodeGen/aarch64-debug-sve-vectorx3-types.c
  clang/test/CodeGen/aarch64-debug-sve-vectorx4-types.c
  clang/test/CodeGen/debug-info-enum.cpp
  clang/test/CodeGen/debug-info.c
  clang/test/CodeGenCXX/debug-info-enum-class.cpp
  clang/test/CodeGenObjC/objc-fixed-enum.m
  lldb/test/Shell/SymbolFile/DWARF/x86/find-basic-variable.cpp
  llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
  llvm/test/CodeGen/MIR/X86/diexpr-win32.mir
  llvm/test/DebugInfo/COFF/types-basic.ll
  llvm/test/DebugInfo/COFF/types-integer-old.ll

Index: llvm/test/DebugInfo/COFF/types-integer-old.ll
===
--- /dev/null
+++ llvm/test/DebugInfo/COFF/types-integer-old.ll
@@ -0,0 +1,77 @@
+; RUN: llc < %s -filetype=obj | llvm-readobj - --codeview | FileCheck %s
+
+; Tests that CodeView integer types are generated even when using Clang's old integer type names.
+
+; C++ source to regenerate:
+; $ cat t.cpp
+; void usevars(long, ...);
+; void f() {
+;   long l1 = 0;
+;   unsigned long l2 = 0;
+;   usevars(l1, l2);
+; }
+; $ clang t.cpp -S -emit-llvm -g -gcodeview -o t.ll  -target x86_64-pc-windows-msvc19.0.23918
+
+; CHECK: LocalSym {
+; CHECK:   Type: long (0x12)
+; CHECK:   VarName: l1
+; CHECK: }
+; CHECK: LocalSym {
+; CHECK:   Type: unsigned long (0x22)
+; CHECK:   VarName: l2
+; CHECK: }
+
+; ModuleID = '/usr/local/google/home/blaikie/dev/scratch/t.cpp'
+source_filename = "/usr/local/google/home/blaikie/dev/scratch/t.cpp"
+target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc19.0.23918"
+
+; Function Attrs: mustprogress noinline optnone uwtable
+define dso_local void @"?f@@YAXXZ"() #0 !dbg !8 {
+entry:
+  %l1 = alloca i32, align 4
+  %l2 = alloca i32, align 4
+  call void @llvm.dbg.declare(metadata i32* %l1, metadata !13, metadata !DIExpression()), !dbg !15
+  store i32 0, i32* %l1, align 4, !dbg !15
+  call void @llvm.dbg.declare(metadata i32* %l2, metadata !16, metadata !DIExpression()), !dbg !18
+  store i32 0, i32* %l2, align 4, !dbg !18
+  %0 = load i32, i32* %l2, align 4, !dbg !19
+  %1 = load i32, i32* %l1, align 4, !dbg !19
+  call void (i32, ...) @"?usevars@@YAXJZZ"(i32 %1, i32 %0), !dbg !19
+  ret void, !dbg !20
+}
+
+; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+declare dso_local void @"?usevars@@YAXJZZ"(i32, ...) #2
+
+attributes #0 = { mustprogress noinline optnone uwtable "frame-pointer"="none" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
+attributes #1 = { nofree nosync nounwind readnone speculatable willreturn }
+attributes #2 = { "frame-pointer"="none" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3, !4, !5, !6}
+!llvm.ident = !{!7}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 14.0.0 (g...@github.com:llvm/llvm-project.git 3709fb72c86bea1f0e6c51ab334ed6417cbe1c07)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "/usr/local/google/home/blaikie/dev/scratch/t.cpp", directory: "/usr/local/google/home/blaikie/dev/llvm/src", checksumkind: CSK_MD5, checksum: "a8e7ccc989ea91d67d3cb95afa046aa5")
+!2 = !{i32 2, !"CodeView", i32 1}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{i32 1, !"wchar_size", i32 2}
+!5 = !{i32 7, !"PIC Level", i32 2}
+!6 = !{i32 7, !"uwtable", i32 1}
+!7 = !{!"clang version 14.0.0 (g...@github.com:llvm/llvm-project.git 3709fb72c86bea1f0e6c51ab334ed6417cbe1c07)"}
+!8 = distinct !DISubprogram(name: "f", linkageName: "?f@@YAXXZ", scope: !9, file: !9, line: 2, type: !10, scopeLine: 2, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !12)
+!9 = !DIFile(filename: "scratch/t.cpp", directory: "/usr/local/google/home/blaikie/dev", checksumkind: CSK_MD5

[Lldb-commits] [PATCH] D111981: [lldb] Fix missing dependency on libc++ from LLDB test suite on non-Darwin platforms

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

So this'll add the right test dependency, if the libcxx project is enabled in 
the build? (& if the build hasn't enabled libcxx, the tests will run, but 
against the system compiler - and if that system compiler happens to default to 
libc++ ( 
https://github.com/llvm/llvm-project/blob/e9e4fc0fd3e0780207c731a1f2b8f6aacd24e8f8/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/list/TestDataFormatterLibcxxList.py#L49
 )) that should remove the "different results if you run `ninja check-lldb` 
before or after `ninja check-libcxx`" issue? Great, thanks!

Hmm, it doesn't appear to be working for me, running this (in a clean build 
directly, synced to 74c4d44d47b282769f6584153e9b433e8e5fa671 
 which 
includes 366fb539485a9753e4a8167fe5140bf4fb00a098 
 (& 
validated by inspecting `lldb/test/CMakeLists.txt`) still shows the tests as 
unsupported:

  CC=clang CXX=clang++ cmake -G Ninja -DCMAKE_BUILD_TYPE=Release 
-DLLVM_ENABLE_WERROR=true 
-DLLVM_ENABLE_PROJECTS='llvm;clang;clang-tools-extra;lld;lldb;cross-project-tests'
 -DLLVM_ENABLE_RUNTIMES='compiler-rt;libcxx;libcxxabi;libunwind' 
-DCMAKE_INSTALL_PREFIX=$HOME/install ~/dev/llvm/src/llvm
  ninja check-lldb-api-functionalities-data-formatter-data-formatter-stl-libcxx

And then running `ninja cxx` and the same check again:

  
  FAIL: lldb-api :: 
functionalities/data-formatter/data-formatter-stl/libcxx/set/TestDataFormatterLibcxxSet.py
 (23 of 23)
   TEST 'lldb-api :: 
functionalities/data-formatter/data-formatter-stl/libcxx/set/TestDataFormatterLibcxxSet.py'
 FAILED 
  Script:
  --
  /usr/bin/python3.9 
/usr/local/google/home/blaikie/dev/llvm/src/lldb/test/API/dotest.py -u CXXFLAGS 
-u CFLAGS --env ARCHIVER=/usr/bin/ar --env OBJCOPY=/usr/bin/objcopy --env 
LLVM_LIBS_DIR=/usr/local/google/home/blaikie/dev/llvm/build/release/./lib 
--arch x86_64 --build-dir 
/usr/local/google/home/blaikie/dev/llvm/build/release/lldb-test-build.noindex 
--lldb-module-cache-dir 
/usr/local/google/home/blaikie/dev/llvm/build/release/lldb-test-build.noindex/module-cache-lldb/lldb-api
 --clang-module-cache-dir 
/usr/local/google/home/blaikie/dev/llvm/build/release/lldb-test-build.noindex/module-cache-clang/lldb-api
 --executable /usr/local/google/home/blaikie/dev/llvm/build/release/./bin/lldb 
--compiler /usr/local/google/home/blaikie/dev/llvm/build/release/./bin/clang 
--dsymutil /usr/local/google/home/blaikie/dev/llvm/build/release/./bin/dsymutil 
--llvm-tools-dir /usr/local/google/home/blaikie/dev/llvm/build/release/./bin 
--lldb-libs-dir /usr/local/google/home/blaikie/dev/llvm/build/release/./lib 
/usr/local/google/home/blaikie/dev/llvm/src/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/set
 -p TestDataFormatterLibcxxSet.py
  --
  Exit Code: 1
  
  Command Output (stdout):
  --
  lldb version 14.0.0 (g...@github.com:llvm/llvm-project.git revision 
74c4d44d47b282769f6584153e9b433e8e5fa671)
clang revision 74c4d44d47b282769f6584153e9b433e8e5fa671
llvm revision 74c4d44d47b282769f6584153e9b433e8e5fa671
  Skipping the following test categories: ['dsym', 'gmodules', 'debugserver', 
'objc']
  
  --
  Command Output (stderr):
  --
  UNSUPPORTED: LLDB 
(/usr/local/google/home/blaikie/dev/llvm/build/release/bin/clang-x86_64) :: 
test_ref_and_ptr_dsym 
(TestDataFormatterLibcxxSet.LibcxxSetDataFormatterTestCase) (test case does not 
fall in any category of interest for this run) 
  FAIL: LLDB 
(/usr/local/google/home/blaikie/dev/llvm/build/release/bin/clang-x86_64) :: 
test_ref_and_ptr_dwarf 
(TestDataFormatterLibcxxSet.LibcxxSetDataFormatterTestCase)
  FAIL: LLDB 
(/usr/local/google/home/blaikie/dev/llvm/build/release/bin/clang-x86_64) :: 
test_ref_and_ptr_dwo (TestDataFormatterLibcxxSet.LibcxxSetDataFormatterTestCase)
  UNSUPPORTED: LLDB 
(/usr/local/google/home/blaikie/dev/llvm/build/release/bin/clang-x86_64) :: 
test_ref_and_ptr_gmodules 
(TestDataFormatterLibcxxSet.LibcxxSetDataFormatterTestCase) (test case does not 
fall in any category of interest for this run) 
  UNSUPPORTED: LLDB 
(/usr/local/google/home/blaikie/dev/llvm/build/release/bin/clang-x86_64) :: 
test_with_run_command_dsym 
(TestDataFormatterLibcxxSet.LibcxxSetDataFormatterTestCase) (test case does not 
fall in any category of interest for this run) 
  FAIL: LLDB 
(/usr/local/google/home/blaikie/dev/llvm/build/release/bin/clang-x86_64) :: 
test_with_run_command_dwarf 
(TestDataFormatterLibcxxSet.LibcxxSetDataFormatterTestCase)
  FAIL: LLDB 
(/usr/local/google/home/blaikie/dev/llvm/build/release/bin/clang-x86_64) :: 
test_with_run_command_dwo 
(TestDataFormatterLibcxxSet.LibcxxSetDataFormatterTestCase)
  UNSUPPORTED: LLDB 
(/usr/local/google/home/blaikie/dev/llvm/build/release/bin/clang-x86_64) :: 
test_with_run_command_gmodules 
(TestDataFormatterLibcxx

[Lldb-commits] [PATCH] D111981: [lldb] Fix missing dependency on libc++ from LLDB test suite on non-Darwin platforms

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D111981#3071378 , @teemperor wrote:

> In D111981#3071349 , @dblaikie 
> wrote:
>
>> So this'll add the right test dependency, if the libcxx project is enabled 
>> in the build? (& if the build hasn't enabled libcxx, the tests will run, but 
>> against the system compiler - and if that system compiler happens to default 
>> to libc++ ( 
>> https://github.com/llvm/llvm-project/blob/e9e4fc0fd3e0780207c731a1f2b8f6aacd24e8f8/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/list/TestDataFormatterLibcxxList.py#L49
>>  )) that should remove the "different results if you run `ninja check-lldb` 
>> before or after `ninja check-libcxx`" issue? Great, thanks!
>>
>> Hmm, it doesn't appear to be working for me, running this (in a clean build 
>> directly, synced to 74c4d44d47b282769f6584153e9b433e8e5fa671 
>>  which 
>> includes 366fb539485a9753e4a8167fe5140bf4fb00a098 
>>  (& 
>> validated by inspecting `lldb/test/CMakeLists.txt`) still shows the tests as 
>> unsupported:
>>
>>   CC=clang CXX=clang++ cmake -G Ninja -DCMAKE_BUILD_TYPE=Release 
>> -DLLVM_ENABLE_WERROR=true 
>> -DLLVM_ENABLE_PROJECTS='llvm;clang;clang-tools-extra;lld;lldb;cross-project-tests'
>>  -DLLVM_ENABLE_RUNTIMES='compiler-rt;libcxx;libcxxabi;libunwind' 
>> -DCMAKE_INSTALL_PREFIX=$HOME/install ~/dev/llvm/src/llvm
>>   ninja 
>> check-lldb-api-functionalities-data-formatter-data-formatter-stl-libcxx
>>
>> And then running `ninja cxx` and the same check again:
>>
>>   
>>   FAIL: lldb-api :: 
>> functionalities/data-formatter/data-formatter-stl/libcxx/set/TestDataFormatterLibcxxSet.py
>>  (23 of 23)
>>    TEST 'lldb-api :: 
>> functionalities/data-formatter/data-formatter-stl/libcxx/set/TestDataFormatterLibcxxSet.py'
>>  FAILED 
>>   Script:
>>   --
>>   /usr/bin/python3.9 
>> /usr/local/google/home/blaikie/dev/llvm/src/lldb/test/API/dotest.py -u 
>> CXXFLAGS -u CFLAGS --env ARCHIVER=/usr/bin/ar --env OBJCOPY=/usr/bin/objcopy 
>> --env 
>> LLVM_LIBS_DIR=/usr/local/google/home/blaikie/dev/llvm/build/release/./lib 
>> --arch x86_64 --build-dir 
>> /usr/local/google/home/blaikie/dev/llvm/build/release/lldb-test-build.noindex
>>  --lldb-module-cache-dir 
>> /usr/local/google/home/blaikie/dev/llvm/build/release/lldb-test-build.noindex/module-cache-lldb/lldb-api
>>  --clang-module-cache-dir 
>> /usr/local/google/home/blaikie/dev/llvm/build/release/lldb-test-build.noindex/module-cache-clang/lldb-api
>>  --executable 
>> /usr/local/google/home/blaikie/dev/llvm/build/release/./bin/lldb --compiler 
>> /usr/local/google/home/blaikie/dev/llvm/build/release/./bin/clang --dsymutil 
>> /usr/local/google/home/blaikie/dev/llvm/build/release/./bin/dsymutil 
>> --llvm-tools-dir /usr/local/google/home/blaikie/dev/llvm/build/release/./bin 
>> --lldb-libs-dir /usr/local/google/home/blaikie/dev/llvm/build/release/./lib 
>> /usr/local/google/home/blaikie/dev/llvm/src/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/set
>>  -p TestDataFormatterLibcxxSet.py
>>   --
>>   Exit Code: 1
>>   
>>   Command Output (stdout):
>>   --
>>   lldb version 14.0.0 (g...@github.com:llvm/llvm-project.git revision 
>> 74c4d44d47b282769f6584153e9b433e8e5fa671)
>> clang revision 74c4d44d47b282769f6584153e9b433e8e5fa671
>> llvm revision 74c4d44d47b282769f6584153e9b433e8e5fa671
>>   Skipping the following test categories: ['dsym', 'gmodules', 
>> 'debugserver', 'objc']
>>   
>>   --
>>   Command Output (stderr):
>>   --
>>   UNSUPPORTED: LLDB 
>> (/usr/local/google/home/blaikie/dev/llvm/build/release/bin/clang-x86_64) :: 
>> test_ref_and_ptr_dsym 
>> (TestDataFormatterLibcxxSet.LibcxxSetDataFormatterTestCase) (test case does 
>> not fall in any category of interest for this run) 
>>   FAIL: LLDB 
>> (/usr/local/google/home/blaikie/dev/llvm/build/release/bin/clang-x86_64) :: 
>> test_ref_and_ptr_dwarf 
>> (TestDataFormatterLibcxxSet.LibcxxSetDataFormatterTestCase)
>>   FAIL: LLDB 
>> (/usr/local/google/home/blaikie/dev/llvm/build/release/bin/clang-x86_64) :: 
>> test_ref_and_ptr_dwo 
>> (TestDataFormatterLibcxxSet.LibcxxSetDataFormatterTestCase)
>>   UNSUPPORTED: LLDB 
>> (/usr/local/google/home/blaikie/dev/llvm/build/release/bin/clang-x86_64) :: 
>> test_ref_and_ptr_gmodules 
>> (TestDataFormatterLibcxxSet.LibcxxSetDataFormatterTestCase) (test case does 
>> not fall in any category of interest for this run) 
>>   UNSUPPORTED: LLDB 
>> (/usr/local/google/home/blaikie/dev/llvm/build/release/bin/clang-x86_64) :: 
>> test_with_run_command_dsym 
>> (TestDataFormatterLibcxxSet.LibcxxSetDataFormatterTestCase) (test case does 
>> not fall in any category of interest for this run) 
>>   FAIL: LLDB 
>> (/usr/local/google/home/blaikie/dev/llvm/build/

[Lldb-commits] [PATCH] D111981: [lldb] Fix missing dependency on libc++ from LLDB test suite on non-Darwin platforms

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

Yeah, applying that patch gets the expected `cannot open shared object file` 
issue for `libc++.so.1`:

  ==
  FAIL: test_with_run_command_dwo 
(TestDataFormatterLibcxxSet.LibcxxSetDataFormatterTestCase)
 Test that that file and class static variables display correctly.
  --
  Traceback (most recent call last):
File 
"/usr/local/google/home/blaikie/dev/llvm/src/lldb/packages/Python/lldbsuite/test/lldbtest.py",
 line 1823, in test_method
  return attrvalue(self)
File 
"/usr/local/google/home/blaikie/dev/llvm/src/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/set/TestDataFormatterLibcxxSet.py",
 line 49, in test_with_run_command
  (self.target, process, _, bkpt) = lldbutil.run_to_source_breakpoint(
File 
"/usr/local/google/home/blaikie/dev/llvm/src/lldb/packages/Python/lldbsuite/test/lldbutil.py",
 line 1007, in run_to_source_breakpoint
  return run_to_breakpoint_do_run(test, target, breakpoint, launch_info,
File 
"/usr/local/google/home/blaikie/dev/llvm/src/lldb/packages/Python/lldbsuite/test/lldbutil.py",
 line 924, in run_to_breakpoint_do_run
  test.fail("Test process is not stopped at breakpoint, but instead in" +
  AssertionError: Test process is not stopped at breakpoint, but instead in 
state 'exited'. Exit code/status: 127. Exit description: None.
  stderr of inferior:
  
/usr/local/google/home/blaikie/dev/llvm/build/release/lldb-test-build.noindex/functionalities/data-formatter/data-formatter-stl/libcxx/set/TestDataFormatterLibcxxSet.test_with_run_command_dwo/a.out:
 error while loading shared libraries: libc++.so.1: cannot open shared object 
file: No such file or directory
  
  Config=x86_64-/usr/local/google/home/blaikie/dev/llvm/build/release/bin/clang
  --


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111981

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

[Lldb-commits] [PATCH] D112163: Enable libc++ in the build for libcxx initializerlist pretty printers

[David Blaikie via Phabricator via lldb-commits]

dblaikie created this revision.
dblaikie added reviewers: teemperor, labath.
dblaikie requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112163

Files:
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/Makefile
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/TestInitializerList.py


Index: 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/TestInitializerList.py
===
--- 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/TestInitializerList.py
+++ 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/TestInitializerList.py
@@ -14,11 +14,7 @@
 
 mydir = TestBase.compute_mydir(__file__)
 
-@skipIfWindows  # libc++ not ported to Windows yet
-@skipIf(compiler="gcc")
-@expectedFailureAll(
-oslist=["linux"],
-bugnumber="fails on clang 3.5 and tot")
+@add_test_categories(["libc++"])
 def test(self):
 """Test that that file and class static variables display correctly."""
 self.build()
Index: 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/Makefile
===
--- 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/Makefile
+++ 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/Makefile
@@ -1,4 +1,6 @@
 CXX_SOURCES := main.cpp
 CXXFLAGS_EXTRAS := -std=c++11
 
+USE_LIBCPP := 1
+
 include Makefile.rules


Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/TestInitializerList.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/TestInitializerList.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/TestInitializerList.py
@@ -14,11 +14,7 @@
 
 mydir = TestBase.compute_mydir(__file__)
 
-@skipIfWindows  # libc++ not ported to Windows yet
-@skipIf(compiler="gcc")
-@expectedFailureAll(
-oslist=["linux"],
-bugnumber="fails on clang 3.5 and tot")
+@add_test_categories(["libc++"])
 def test(self):
 """Test that that file and class static variables display correctly."""
 self.build()
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/Makefile
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/Makefile
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/Makefile
@@ -1,4 +1,6 @@
 CXX_SOURCES := main.cpp
 CXXFLAGS_EXTRAS := -std=c++11
 
+USE_LIBCPP := 1
+
 include Makefile.rules
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112163: Enable libc++ in the build for libcxx initializerlist pretty printers

[David Blaikie via Phabricator via lldb-commits]

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

Looks like this test, when it was introduced/substantially refactored in D9426 
 - what was missing compared to all the other 
tests updated to run on linux, was the Makefile change to use libc++, so it was 
failing in places where libc++ wasn't the default. Fix that and it seems to 
pass for me, at least. Not sure if we need to keep some of the other expected 
failures (on some particular subset of clang, etc?)

Also there's a few other of these pretty printer tests that are still 
classified as skip-on-gcc, though at least some have been reclassified as 
passing-on-gcc in changes since D9426 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112163

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

[Lldb-commits] [PATCH] D112165: Cleanup a few more PR36048 skips

[David Blaikie via Phabricator via lldb-commits]

dblaikie created this revision.
dblaikie added a reviewer: aprantl.
dblaikie requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Hopefully these were also fixed bi r343545 /
7fd4513920d2fed533ad420976529ef43eb42a35


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112165

Files:
  
lldb/test/API/functionalities/data-formatter/data-formatter-cpp/TestDataFormatterCpp.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-globals/TestDataFormatterGlobals.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/list/TestDataFormatterLibcxxList.py


Index: 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/list/TestDataFormatterLibcxxList.py
===
--- 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/list/TestDataFormatterLibcxxList.py
+++ 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/list/TestDataFormatterLibcxxList.py
@@ -27,8 +27,6 @@
  '// Set fourth break point at this line.')
 
 @add_test_categories(["libc++"])
-@skipIf(debug_info="gmodules",
-bugnumber="https://bugs.llvm.org/show_bug.cgi?id=36048";)
 def test_with_run_command(self):
 """Test that that file and class static variables display correctly."""
 self.build()
Index: 
lldb/test/API/functionalities/data-formatter/data-formatter-globals/TestDataFormatterGlobals.py
===
--- 
lldb/test/API/functionalities/data-formatter/data-formatter-globals/TestDataFormatterGlobals.py
+++ 
lldb/test/API/functionalities/data-formatter/data-formatter-globals/TestDataFormatterGlobals.py
@@ -20,8 +20,6 @@
 # Find the line number to break at.
 self.line = line_number('main.cpp', '// Set break point at this line.')
 
-@skipIf(debug_info="gmodules",
-bugnumber="https://bugs.llvm.org/show_bug.cgi?id=36048";)
 def test_with_run_command(self):
 """Test that that file and class static variables display correctly."""
 self.build()
Index: 
lldb/test/API/functionalities/data-formatter/data-formatter-cpp/TestDataFormatterCpp.py
===
--- 
lldb/test/API/functionalities/data-formatter/data-formatter-cpp/TestDataFormatterCpp.py
+++ 
lldb/test/API/functionalities/data-formatter/data-formatter-cpp/TestDataFormatterCpp.py
@@ -20,8 +20,6 @@
 # Find the line number to break at.
 self.line = line_number('main.cpp', '// Set break point at this line.')
 
-@skipIf(debug_info="gmodules",
-bugnumber="https://bugs.llvm.org/show_bug.cgi?id=36048";)
 def test_with_run_command(self):
 """Test that that file and class static variables display correctly."""
 self.build()


Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/list/TestDataFormatterLibcxxList.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/list/TestDataFormatterLibcxxList.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/list/TestDataFormatterLibcxxList.py
@@ -27,8 +27,6 @@
  '// Set fourth break point at this line.')
 
 @add_test_categories(["libc++"])
-@skipIf(debug_info="gmodules",
-bugnumber="https://bugs.llvm.org/show_bug.cgi?id=36048";)
 def test_with_run_command(self):
 """Test that that file and class static variables display correctly."""
 self.build()
Index: lldb/test/API/functionalities/data-formatter/data-formatter-globals/TestDataFormatterGlobals.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-globals/TestDataFormatterGlobals.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-globals/TestDataFormatterGlobals.py
@@ -20,8 +20,6 @@
 # Find the line number to break at.
 self.line = line_number('main.cpp', '// Set break point at this line.')
 
-@skipIf(debug_info="gmodules",
-bugnumber="https://bugs.llvm.org/show_bug.cgi?id=36048";)
 def test_with_run_command(self):
 """Test that that file and class static variables display correctly."""
 self.build()
Index: lldb/test/API/functionalities/data-formatter/data-formatter-cpp/TestDataFormatterCpp.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-cpp/TestDataFormatterCpp.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-cpp/TestDataFormatterCpp.py
@@ -20,8 +20,6 @@
 # Find the line number to break at.
 self.line = line_number('main.cpp', '// Set break point at this line.'

[Lldb-commits] [PATCH] D112163: Enable libc++ in the build for libcxx initializerlist pretty printers

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D112163#3077112 , @teemperor wrote:

> LGTM, thanks!
>
> Tests with libc++ as a category are not run on Windows or when GCC is the 
> test compiler, so those decorators are redundant. Removing the Clang XFAIL 
> also seems fine, I would just see if any matrix bot complains and then 
> re-apply it with a proper minimum required version.

"matrix bot"? One of the buildbots/group of them?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112163

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

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

Does this sort of thing itself get tested? (like this one had a test: 
https://reviews.llvm.org/D111978 but not sure how much that generalizes/whether 
there are different parts of the infrastructure are more or less testable)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112212

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

[Lldb-commits] [PATCH] D112163: Enable libc++ in the build for libcxx initializerlist pretty printers

[David Blaikie via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd723ad5bcf71: Enable libc++ in the build for libcxx 
initializerlist pretty printers (authored by dblaikie).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112163

Files:
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/Makefile
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/TestInitializerList.py


Index: 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/TestInitializerList.py
===
--- 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/TestInitializerList.py
+++ 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/TestInitializerList.py
@@ -14,11 +14,7 @@
 
 mydir = TestBase.compute_mydir(__file__)
 
-@skipIfWindows  # libc++ not ported to Windows yet
-@skipIf(compiler="gcc")
-@expectedFailureAll(
-oslist=["linux"],
-bugnumber="fails on clang 3.5 and tot")
+@add_test_categories(["libc++"])
 def test(self):
 """Test that that file and class static variables display correctly."""
 self.build()
Index: 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/Makefile
===
--- 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/Makefile
+++ 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/Makefile
@@ -1,4 +1,6 @@
 CXX_SOURCES := main.cpp
 CXXFLAGS_EXTRAS := -std=c++11
 
+USE_LIBCPP := 1
+
 include Makefile.rules


Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/TestInitializerList.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/TestInitializerList.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/TestInitializerList.py
@@ -14,11 +14,7 @@
 
 mydir = TestBase.compute_mydir(__file__)
 
-@skipIfWindows  # libc++ not ported to Windows yet
-@skipIf(compiler="gcc")
-@expectedFailureAll(
-oslist=["linux"],
-bugnumber="fails on clang 3.5 and tot")
+@add_test_categories(["libc++"])
 def test(self):
 """Test that that file and class static variables display correctly."""
 self.build()
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/Makefile
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/Makefile
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/Makefile
@@ -1,4 +1,6 @@
 CXX_SOURCES := main.cpp
 CXXFLAGS_EXTRAS := -std=c++11
 
+USE_LIBCPP := 1
+
 include Makefile.rules
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112340: [lldb/Formatters] Remove space from vector type string summaries (NFCI)

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

Sorry I missed this - are these tested anywhere/should I have been able to 
discover if these needed to be changed before I made the change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112340

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

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D112212#3081828 , @JDevlieghere 
wrote:

> In D112212#3080491 , @teemperor 
> wrote:
>
>> This LGTM, but `shlex.join` is actually Py3 exclusive and I don't think 
>> there is a good Py2 replacement. I think we're just in time for the Py2->3 
>> migration according to the timeline Jonas posted last year 
>> , so 
>> let's use this patch to actually do that? Then we can also get rid of all 
>> the `six` stuff etc.
>>
>> Let's see if Jonas has any objections against dropping Py2 with this, 
>> otherwise this is good to go.
>
> We're planning to branch from open source on October 26th. If there's no 
> urgency, it would really be great if we can hold off breaking Py2 until then.
>
> I'm all in favor for getting rid of Python 2 support, but sweeping changes 
> like dropping the `six` stuff will introduce a lot of headaches (merge 
> conflicts) for us. If we could postpone that for another release that would 
> save us a bunch of engineering time.

No judgment (I think it's a reasonable request to punt a patch like this a few 
days if it helps out major contributors) - but I'm curious/just not quite 
wrapping my head around: Why would it be easier if this sort of patch went in 
after you branch? I'd have thought it'd be easier if it goes in before the 
branch. That way when you're backporting patches from upstream after the branch 
there will be fewer unrelated changes/merge conflicts, yeah?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112212

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