[Lldb-commits] [PATCH] D47492: DWARFUnit::m_die_array swap()->shrink_to_fit()

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In https://reviews.llvm.org/D47492#1120599, @jankratochvil wrote:

> FYI I have filed it for libstdc++ but I did not really understand their 
> reaction: Bug 86013 - std::vector::shrink_to_fit() could sometimes use 
> realloc() 


Happy to help explain it - which part(s) are you having a bit of trouble with?

It seems like the main one is that the implementation can't be sure that malloc 
was used to allocate the memory - since the global allocation function can be 
replaced & there's no convenient way to detect that.


Repository:
  rL LLVM

https://reviews.llvm.org/D47492



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

[Lldb-commits] [PATCH] D49309: No longer pass a StringRef to the Python API

[David Blaikie via Phabricator via lldb-commits]

dblaikie added subscribers: teemperor, dblaikie.
dblaikie added a comment.

If  the implementation of a function requires a string - it should probably
accept string (not a StringRef) as a parameter - to avoid back-and-forth
conversions in cases where the caller already has a string to use.


Repository:
  rL LLVM

https://reviews.llvm.org/D49309



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

[Lldb-commits] [PATCH] D49411: Move from StringRef to std::string in the ScriptInterpreter API

[David Blaikie via Phabricator via lldb-commits]

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

Seems good to me - though I haven't looked too closely/don't know this code 
terribly well (so you're welcome to wait if you know someone else more 
knowledgable might take a look too - or if you're fairly confident, commit & 
they can always follow up in post-commit)


https://reviews.llvm.org/D49411



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

[Lldb-commits] [PATCH] D49411: Move from StringRef to std::string in the ScriptInterpreter API

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In https://reviews.llvm.org/D49411#1164680, @labath wrote:

> Normally this would be clearly a good thing, but the added complication here 
> is that this function is part of a class hierarchy, and so  this way you are 
> forcing every implementation to take a std::string, even though only one of 
> them cares about null-termination.
>
> In performance-critical code, llvm would use `llvm::Twine` as an argument, 
> which is able to avoid copies if the input string happens to be 
> null-terminated (`toNullTerminatedStringRef`). However, this code is hardly 
> that critical (and ScriptInterpreterPython is the only non-trivial class in 
> the hierarchy), so I don't think it really matters what you do here.


Fair points all, Pavel - thanks for chiming in!


https://reviews.llvm.org/D49411



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

[Lldb-commits] [PATCH] D51208: [DWARF] Fix dwarf5-index-is-used.cpp

[David Blaikie via Phabricator via lldb-commits]

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

In https://reviews.llvm.org/D51208#1212320, @labath wrote:

> As far as the strict intention of this test goes, the change is fine, as it 
> is meant to check that the accelerator tables get used *when* they are 
> generated. How do you achieve them being generated is not important.
>
> However, I am not sure now under what circumstances will the accelerator 
> tables be emitted in the first place. David, does this mean that we will now 
> not emit .debug-names even if one specifies `-glldb`? Was that intentional?


Not especially intentional - but I clearly didn't give it quite enough thought. 
Mostly I was modelling the behavior of GCC (no pubnames by default - you can 
opt-in to them (& split-dwarf opts in by default, but one thing GCC didn't do 
was allow them to be turned off again - which is what I wanted)).

As for the default behavior for DWARFv5 - have you run much in the way of 
performance numbers on how much debug_names speeds things up? From what I could 
see with a gdb-index (sort of like debug_names - but linker generated, so it's 
a single table for the whole program) it didn't seem to take GDB long to 
parse/build up its own index compared to using the one in the file. So it 
seemed to me like the extra link time, object size, etc, wasn't worth it in the 
normal case. The really bad case for me is split-DWARF (worse with a 
distributed filesystem) - where the debugger has to read all the .dwo files & 
might have a high latency filesystem for each file it reads... super slow. But 
if the debug info was either in the executable (not split) or in a DWP (split 
then packaged), it seemed pretty brief.

But if LLDB has different performance characteristics, or the default should be 
different for other reasons - I'm fine with that. I think I left it on for 
Apple so as not to mess with their stuff because of the MachO/dsym sort of 
thing that's a bit different from the environments I'm looking at.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D51208



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

[Lldb-commits] [PATCH] D51208: [DWARF] Fix dwarf5-index-is-used.cpp

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

>> But if LLDB has different performance characteristics, or the default should 
>> be different for other reasons - I'm fine with that. I think I left it on 
>> for Apple so as not to mess with their stuff because of the MachO/dsym sort 
>> of thing that's a bit different from the environments I'm looking at.
> 
> These are the numbers from my llvm-dev email in June:
> 
>> setting a breakpoint on a non-existing function without the use of
>>  accelerator tables:
>>  real0m5.554s
>>  user0m43.764s
>>  sys 0m6.748s
>> 
>> setting a breakpoint on a non-existing function with accelerator tables:
>>  real0m3.517s
>>  user0m3.136s
>>  sys 0m0.376s
> 
> This is an extreme case,

What was being tested here? Is it a realistic case (ie: not a pathalogical case 
with an absurd number of symbols, etc)? Using ELF? Fission or not?

How's it compare to GDB performance, I wonder? Perhaps LLDB hasn't been 
optimized for the non-indexed case and could be - though that'd still 
potentially motivate turning on indexes by default for LLDB until someone has a 
motivation to do any non-indexed performance tuning/improvements.

> because practically the only thing we are doing is building the symbol index, 
> but it's nice for demonstrating the amount of work that lldb needs to do 
> without it. In practice, the ratio will not be this huge most of the time, 
> because we will usually find some matches, and then will have to do some 
> extra work, which will add a constant overhead to both sides. However, this 
> means that the no-accel case will take even longer. I am not sure how this 
> compares to gdb numbers, but I think the difference here is significant.
> 
> Also, I am pretty sure the Apple folks, who afaik are in the process of 
> switching to debug_names, will want to have them on by default for their 
> targets (ping @aprantl, @JDevlieghere). I think the cleanest way (and one 
> that best reflects the reality) to achieve that would be to have `-glldb` 
> imply `-gpubnames`. As for whether we should emit debug_names for DWARF 5 by 
> default (-gdwarf-5 => -gpubnames) is a more tricky question, and I don't have 
> a clear opinion on that (however, @probinson might).
> 
> (In either case, I agree that there are circumstances in which having 
> debug_names is not beneficial, so having a flag to control them is a good 
> idea).

*nod* Happy if you want to (or I can) have clang set pubnames on by default 
when tuning for LLDB, while allowing -gno-pubnames to turn them off. (& maybe 
we should have another alias for that flag, since debug_names isn't "pubnames", 
but that's pretty low-priority)


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D51208



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

[Lldb-commits] [PATCH] D45628: [LLDB] Support compressed debug info sections (.zdebug*)

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In https://reviews.llvm.org/D45628#1067375, @aprantl wrote:

> Which compilers / platforms generate / support this? Is this an ELF-only 
> feature?


Clang/LLVM do (-Wa,--compress-debug-sections). Yeah, it's ELF only so far as I 
know. There's a couple of variations (-compress-debug-sections=zlib or 
zlib-gnu) the ".zdebug_foo" is zlib-gnu style, but a more modern variant (zlib) 
uses a section attribute SHF_COMPRESSED instead of the name mangling.

In https://reviews.llvm.org/D45628#1067382, @clayborg wrote:

> If this is for current and future debugging, it would be nice to change the 
> tool to just use the normal .debug prefixes and just specify SHF_COMPRESSED???


At least for now, zlib-gnu style is used at Google - I don't actually know what 
it'd cost to switch to the more modern zlib (using SHF_COMPRESSED) style. No 
doubt there are a variety of tools that may not have been updated/improved to 
support the new format - hard to say.


https://reviews.llvm.org/D45628



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

[Lldb-commits] [PATCH] D35740: Fix PR33875 by distinguishing between DWO and clang modules

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

(Presumably this could use a test case?)


Repository:
  rL LLVM

https://reviews.llvm.org/D35740



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

[Lldb-commits] [PATCH] D154907: [LLDB] Fix buffer overflow problem in DWARFExpression::Evaluate (2nd attempt)

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D154907#4487335 , @jasonmolenda 
wrote:

> This looks good to me, thanks for digging in Caroline!  Is there a naughty 
> compiler emitting this, or are we mis-parsing somehow?



In D154907#4487523 , @cmtice wrote:

> Thanks Jason. I think the compiler is generating some bad DWARF.  David 
> Blaikie was investigating that on our end, but he's on vacation this week.

Yep - something in libc++ built with clang:

DW_AT_name
("third_party/llvm/llvm-project/libcxx/src/ios.instantiations.cpp")
   [0x2b32cf96, 0x2b32d0e9): DW_OP_breg6 
RBP-52, DW_OP_deref_size 0x10, DW_OP_stack_value)
   [0x2b32d116, 0x2b32d265): DW_OP_breg6 
RBP-56, DW_OP_deref_size 0x10, DW_OP_stack_value)
  DW_AT_location  (DW_OP_fbreg -56, DW_OP_deref_size 
0x10, DW_OP_stack_value)
   [0x2b331396, 0x2b3314e6): DW_OP_breg6 
RBP-52, DW_OP_deref_size 0x10, DW_OP_stack_value)
   [0x2b331516, 0x2b331662): DW_OP_breg6 
RBP-56, DW_OP_deref_size 0x10, DW_OP_stack_value)
  DW_AT_location  (DW_OP_fbreg -56, DW_OP_deref_size 
0x10, DW_OP_stack_value)
  --
DW_AT_name
("third_party/llvm/llvm-project/libcxx/src/locale.cpp")
   [0x2b34146b, 0x2b341623): DW_OP_breg6 
RBP-48, DW_OP_deref_size 0x10, DW_OP_stack_value
   [0x2b3416d7, 0x2b3417e7): DW_OP_breg6 
RBP-48, DW_OP_deref_size 0x10, DW_OP_stack_value
   [0x2b342b2b, 0x2b342cdd): DW_OP_breg6 
RBP-48, DW_OP_deref_size 0x10, DW_OP_stack_value
   [0x2b342d96, 0x2b342ea6): DW_OP_breg6 
RBP-48, DW_OP_deref_size 0x10, DW_OP_stack_value

I haven't isolated this - got to extract/figure out how our libc++ is built, 
etc.




Comment at: lldb/source/Expression/DWARFExpression.cpp:1082-1089
 void *src = (void *)stack.back().GetScalar().ULongLong();
 intptr_t ptr;
 ::memcpy(&ptr, src, sizeof(void *));
 // I can't decide whether the size operand should apply to the bytes in
 // their
 // lldb-host endianness or the target endianness.. I doubt this'll ever
 // come up but I'll opt for assuming big endian regardless.

Just as an aside - isn't this code doing an illegal load widening? If the 
pointer pointed to the end of a page or something, and asked for only one byte 
- reading extra bytes would be bad (similarly would cause a segfault/UB/etc), 
right?

(& I'm not sure I understand the comment about endianness - the operation reads 
that many bytes from the given address)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154907

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

[Lldb-commits] [PATCH] D154907: [LLDB] Fix buffer overflow problem in DWARFExpression::Evaluate (2nd attempt)

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: lldb/source/Expression/DWARFExpression.cpp:1082-1089
 void *src = (void *)stack.back().GetScalar().ULongLong();
 intptr_t ptr;
 ::memcpy(&ptr, src, sizeof(void *));
 // I can't decide whether the size operand should apply to the bytes in
 // their
 // lldb-host endianness or the target endianness.. I doubt this'll ever
 // come up but I'll opt for assuming big endian regardless.

dblaikie wrote:
> Just as an aside - isn't this code doing an illegal load widening? If the 
> pointer pointed to the end of a page or something, and asked for only one 
> byte - reading extra bytes would be bad (similarly would cause a 
> segfault/UB/etc), right?
> 
> (& I'm not sure I understand the comment about endianness - the operation 
> reads that many bytes from the given address)
oh, guess I also mentioned this here: 
https://reviews.llvm.org/D153840#inline-1494202


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154907

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

[Lldb-commits] [PATCH] D154907: [LLDB] Fix buffer overflow problem in DWARFExpression::Evaluate (2nd attempt)

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a subscriber: jmorse.
dblaikie added a comment.

Simple clang example that produces this invalid DWARF:

  void b(double);
  void c();
  void e(double e) {
c();
b(e);
  }

  $ clang-tot x.ii -g -c -o - -O3 | llvm-dwarfdump-tot - | grep DW_OP_deref_size
   [0x0006, 0x0016): DW_OP_breg7 
RSP+0, DW_OP_deref_size 0x10, DW_OP_stack_value)

This seems to be created here: 
https://github.com/llvm/llvm-project/blob/dd84f5f91c6b234a2f188b6acf8557cae81b8a53/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp#L1281
 - so I'll file a bug for @jmorse to take a look at, perhaps.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154907

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

[Lldb-commits] [PATCH] D154907: [LLDB] Fix buffer overflow problem in DWARFExpression::Evaluate (2nd attempt)

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

Filed https://github.com/llvm/llvm-project/issues/64093


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154907

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

[Lldb-commits] [PATCH] D157609: [lldb] Add more ways to find split DWARF files

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

Any chance we could simplify this situation and have dwo searches use exactly 
the same/shared logic as source file searches? It seems like the dwarf spec 
intent was for that to be the case.

I don't mind adding more ways to find things, but think it'd be useful if we 
only had one set of rules instead of two, to prevent divergence.

(The script idea from Greg sounds ok too (but ideally, also in the context of 
one search system for both source and dwo) - but yeah, maybe invasive enough 
that it merits a separate/broader design discussion)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157609

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

[Lldb-commits] [PATCH] D157609: [lldb] Add more ways to find split DWARF files

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D157609#4645298 , @clayborg wrote:

> In D157609#4644194 , @DavidSpickett 
> wrote:
>
>>> Any chance we could simplify this situation and have dwo searches use 
>>> exactly the same/shared logic as source file searches?
>>
>> I'm not sure what exactly this means, can you clarify?
>>
>> I know that DWP and DWO have different search functions, that could 
>> certainly be unified. Not sure how source files work.
>
> Source file search paths and debug info search paths are different in LLDB. 
> GDB had one setting IIRC that handled both. These settings have been in LLDB 
> for a while so I wouldn't recommend changing this up at this point.

I don't think that's a great motivation to diverge them further.

We could make the DWO search fallback to the source search - and add new 
features to the source search if needed. That'd at least align things a bit 
closer, rather than diverging further.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157609

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

[Lldb-commits] [PATCH] D115313: [lldb/Target] Slide source display for artificial location at function start

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

Side note: The src_file is not to be trusted/used either - once line 0 is 
specified, nothing else in that line entry is valid. LLVM lets the previous 
line entries file persist because this reduces encoding size (by not having to 
switch all the fields in the line table - only the line number - back and forth 
over a line number 0 area). eg: previous entry in the line table might be from 
a #include of some code (as in clang/llvm's use of .def files, for instance) 
into a function, or from some inlining above the line 0 region, etc.

So maybe "artificial location in function " might be suitable? (the actual 
code at line 0 might still be from some inlining (LLVM does try to scope it - 
so the instruction should have the nearest common scope (in terms of lexical 
scopes or inlined functions) so if A has B inlined, B has C and D inlined into 
it and some code is commoned between C and D, it should be attributed to the 
inlined region of B - but if it's hoisted out of a basic block, I don't think 
we can properly attribute it to any scope, and so we'd have to attribute it to 
A in the scope DIE information (in all these cases it'd still have line 0, 
though).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115313

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

[Lldb-commits] [PATCH] D113604: [lldb][NFC] Format lldb/include/lldb/Symbol/Type.h

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


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

https://reviews.llvm.org/D113604

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

[Lldb-commits] [PATCH] D118812: [lldb] Add a setting to skip long mangled names

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

Any chance you might want a limit on the size of the demangled name too? (might 
be worth considering what the most densely encoded mangled name is (ie: what's 
the longest name that could be produced by a 10k long mangled name? and see if 
that's worth having another cutoff for)


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

https://reviews.llvm.org/D118812

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

[Lldb-commits] [PATCH] D118812: [lldb] Add a setting to skip long mangled names

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a subscriber: rsmith.
dblaikie added a comment.

In D118812#3291303 , @jingham wrote:

> In D118812#3291109 , @dblaikie 
> wrote:
>
>> Any chance you might want a limit on the size of the demangled name too? 
>> (might be worth considering what the most densely encoded mangled name is 
>> (ie: what's the longest name that could be produced by a 10k long mangled 
>> name? and see if that's worth having another cutoff for)
>
> Ironically, lldb seldom cares about most of the goo in these long demangled 
> names.  At this point, we are building up our fast-lookup "name indexes".  We 
> really only care about extracting the fully scoped names of the methods.  
> When we get around to doing smart matching on overloads, we can still pull 
> out all the matches to the method name, and then do the overload match on the 
> results.  That should be sufficiently efficient, and obviate the need to do 
> any fancy indexing based on overloads.  So most of the work of demangling 
> these names is not being used anyway.
>
> So what would be the better solution for lldb on the demangling front would 
> be a way to tell the demangler "only extract the full method name, and don't 
> bother producing the argument list or return values".  But I have no idea how 
> easy that would be in the demangler.

I think there's an API level of the demangler in LLVM designed for rewriting 
demangled names (@rsmith created/implemented that, I think) - I'm not sure if 
it's structured to allow lazy parsing/stopping after you get the base name, for 
instance, but maybe...


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

https://reviews.llvm.org/D118812

___
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] D123580: [libc++] Use bit field for checking if string is in long or short mode

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: libcxx/utils/gdb/libcxx/printers.py:192
 
 class StdStringPrinter(object):
 """Print a std::string."""

philnik wrote:
> jgorbe wrote:
> > Mordante wrote:
> > > philnik wrote:
> > > > Mordante wrote:
> > > > > philnik wrote:
> > > > > > Mordante wrote:
> > > > > > > Does this also break the LLDB pretty printer?
> > > > > > Probably. Would be nice to have a test runner for that.
> > > > > I already planned to look into that, D97044#3440904 ;-)
> > > > Do you know where I would have to look to know what the LLDB pretty 
> > > > printers do?
> > > Unfortunately no. @jingham seems to be the Data formatter code owner.
> > There was a recent lldb change fixing prettyprinters after a similar change 
> > to string: 
> > https://github.com/llvm/llvm-project/commit/45428412fd7c9900d3d6ac9803aa7dcf6adfa6fe
> > 
> > If the gdb prettyprinter needed fixing for this change, chances are that 
> > lldb will need a similar update too.
> Could someone from #lldb help me figure out what to change in the pretty 
> printers? I looked at the file, but I don't really understand how it works 
> and TBH I don't really feel like spending a lot of time figuring it out. If 
> nobody says anything I'll land this in a week.
> 
> As a side note: it would be really nice if there were a few more comments 
> inside `LibCxx.cpp` to explain what happens there. That would make fixing the 
> pretty printer a lot easier. The code is probably not very hard (at least it 
> doesn't look like it), but I am looking for a few things that I can't find 
> and I have no idea what some of the things mean.
Looks like something around 
https://github.com/llvm/llvm-project/blob/2e6ac54cf48aa04f7b05c382c33135b16d3f01ea/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp#L597
 (& the similar masking in the `else` block a few lines down) - I guess a 
specific lookup for the new field would be needed, rather than the bitmasking.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123580

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

[Lldb-commits] [PATCH] D124370: [lldb] Fix PR54761

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

I take it no tests fail upon removing this condition? Any hints in version 
history about its purpose?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124370

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

(sorry, this slipping through somehow - it's on my radar now)


Repository:
  rG LLVM Github Monorepo

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#3424654 , @JDevlieghere 
wrote:

> The ConstString/StringPool is pretty hot so I'm very excited about making it 
> faster.
>
> I'm slightly concerned about the two hash values (the "full" hash vs the 
> single byte one). That's not something that was introduced in this patch, but 
> passing it around adds an opportunity to get it wrong.
>
> I'm wondering if we could wrap this in a struct and pass that around:
>
>   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)) {}
>   }
>
> It looks like we always need both except in the StringMap implementation, but 
> if I'm reading the code correctly we'll have constructed it in ConstString 
> already anyway?

I'm sort of with you here - I don't think this API feature needs to affect lldb 
much - the scope of these externally-computed hashes is small, but the 
StringMap API could be more robust/less error prone by having a struct like 
this. Specifically `StringMap::hash` could/should return an immutable struct 
that contains the `StringRef` and the hash (not that that's bulletproof - the 
data beneath the `StringRef` could still be modified externally - but the same 
is true of any hashing data structure - the keys might be shallow or have 
mutable state, etc) - and that immutable object must be passed back to the 
hash-providing insertion functions. (these functions could/should probably 
still then assert that the hash is correct in case of that underlying mutation 
- though someone could argue that's overkill and I'd be open to having that 
discussion).

By immutable I mean that the caller can't go and modify either the `StringRef` 
or hash to make these out of sync. These handles are probably still copy 
assignable. 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.




Comment at: lldb/source/Utility/ConstString.cpp:107-109
 llvm::sys::SmartScopedReader rlock(m_string_pools[h].m_mutex);
-auto it = m_string_pools[h].m_string_map.find(string_ref);
+auto it = m_string_pools[h].m_string_map.find(string_ref, string_hash);
 if (it != m_string_pools[h].m_string_map.end())

total aside, but it might be nice to refactor `m_string_pools[h]` out into a 
named variable here - accessing it 3 times and in a context where it's super 
important that it's the same thing in all 3 places, seems like it'd be easier 
to read with a named variable - might make it clear which things need to happen 
under the lock and which ones don't, etc.

(oh, and the 4th and 5th use a few lines down - and I think that'd be the only 
uses of `h`, so `h` could go away and `hash` could be called inline in the 
array index instead, maybe?)


Repository:
  rG LLVM Github Monorepo

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] D124370: [lldb/DWARF] Fix linking direction in CopyUniqueClassMethodTypes

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D124370#3474824 , @labath wrote:

> In D124370#3472394 , @clayborg 
> wrote:
>
>> So I believe the reason we need to be able to add methods to a class is for 
>> templates. Templates in DWARF are really poorly emitted all in the name of 
>> saving space. There is no full definition of template classes that get 
>> emitted, just a class definition with _only_ the methods that the user used 
>> in the current compile unit. DWARF doesn't really support emitting a 
>> templatized definition of a class in terms of a type T, it will always emit 
>> instantiations of the type T directly. So in LLDB we must create a template 
>> class like "std::vector" and say it has no member functions, and then 
>> add each member function as a type specific specialization due to how the 
>> types must be created in the clang::ASTContext.
>>
>> in one DWARF compile unit you end up having say "std::vector::clear()" 
>> and in another you would have "std::vector::erase()". In order to make 
>> the most complete definition of template types we need to find all 
>> "std::vector" definitions and manually add any method that we haven't 
>> already seen, otherwise we end up not being able to call methods that might 
>> exist. Of course calling template functions is already fraught with danger 
>> since most are inlined if they come from the STL, but if the user asks for 
>> the class definition for "std::vector", we should show all that we can. 
>> Can you make sure this patch doesn't hurt that functionality? We must have a 
>> test for it.
>>
>> So we can't just say we will accept just one class definition if we have 
>> template types. Given this information, let me know what you think of your 
>> current patch
>
> I think I'm confused. I know we're doing some funny stuff with class 
> templates, and I'm certain I don't fully understand how it works. However, I 
> am also pretty sure your description is not correct either.  A simple snippet 
> like `std::vector v;` will produce a definition for the `vector` 
> class, and that definition will include the declarations for `erase`, 
> `clear`, and any other non-template member function, even though the code 
> definitely does not call them. And this makes perfect sense to me given how 
> DWARF (and clang) describes only template instantiations -- so the template 
> instantiation `vector` is treated the same way as a class `vector_int` 
> with the same interface.
>
> What you're describing resembles the treatment of template member functions 
> -- those are only emitted in the compile units that they are used. This is 
> very unfortunate for us, though I think it still makes sense from the DWARF 
> perspective. However:
> a) In line with the previous paragraph -- this behavior is the same for 
> template **class** instantiations and regular classes. IOW, the important 
> question is whether the method is a template, not whether its enclosing class 
> is
> b) (precisely for this reason) we currently  completely ignore 
> 
>  member function templates when parsing classes
> Given all of that,  I don't see how templates are relevant for this patch. 
> *However*, please read on.

Yep, that mostly lines up with my understand.

Re: member function templates: These aren't the only things that vary between 
copies of a type in the DWARF. The other things that vary are implicit special 
member functions ({default,move,copy} ctor, dtor, {copy,move} assignment 
operator}) - these will only be present when instantiated - and nested types 
(though that's perhaps less of an issue for LLDB, not sure). I have made an 
argument that functions with "auto" return type should be treated this way 
(omitted unless the definition is instantiated and the return type is 
deduced/resolved) too - but for now they are not (they are always included, 
albeit with the "auto" return type on the declaration, and then the definition 
DIE has the real/deduced return type).

> In D124370#3472555 , @dblaikie 
> wrote:
>
>> I take it no tests fail upon removing this condition? Any hints in version 
>> history about its purpose?
>
> No tests broke with that. My first archaeology expedition did not find 
> anything, but now I've tried it once more, and I found D13224 
> . That diff is adding this condition (to 
> support -flimit-debug-info, no less) and it even comes with a test. That test 
> still exists, but it was broken over time (in several ways). Once I fix it so 
> that it tests what it was supposed to test, I see that my patch breaks it.
>
> I'm now going to try to figure out how does that code actually work...

Should the cleanups you found for the previous test case be included in this 
patch (or a separate pre/post-co

[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] D139379: [llvm][dwwarf] Change CU/TU index to 64-bit

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

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?)




Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFUnitIndex.h:115
+private:
+  uint64_t Fields[2];
+

How come this became an array? Rather than keeping it as two named fields?


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] D139649: [lldb] Make ParseTemplateParameterInfos return false if there are no template params

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1251
+  if (has_template_params &&
+  ParseTemplateParameterInfos(die, template_param_infos)) {
+template_function_decl = m_ast.CreateFunctionDeclaration(

This part changes behavior, yeah? (previously the code was only conditional on 
`has_template_params` and is now conditional on both that and there actually 
being template parameter DIEs) Was that intended? If so, is there any testing 
that could be done to demonstrate the change in behavior?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139649

___
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] D148776: [Modules] Move modulemaps to header search directories. NFC intended.

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

Got a link to a design discussion motivating this change?

I'd have thought it made sense to put modulemaps in subdirectories - since they 
cover the whole directory, putting them in the root of an include path would be 
problematic if there are multiple distinct projects in that same path. And I 
didn't think this involved searching through subdirectories, but walking up 
parent directories from the included file...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148776

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

[Lldb-commits] [PATCH] D152708: [RFC][Draft] Enable primitive support for Two-Level Line Tables in LLVM

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

Really appreciate you providing this prototype.

> This solves one of the current weaknesses in DWARF debugging, which is the 
> inability to set breakpoints or step onto inlined callsites. However, there 
> are other proposals (such as Location View Numbering) that could achieve the 
> same thing, and the costs of TLLT in storage size and design complexity may 
> make it a non-ideal solution to this problem.

FWIW, one of the major motivations for this was not only to include the stop 
points, but also to allow symbolizing including stack frames - so you'd need to 
know that all the inlined instructions come from the inlined call site. I 
think, maybe, my vague understanding of Location View Numbering wouldn't cover 
that case, and only covers the "how do I describe the variable when I'm stopped 
at this synthetic location that exists just before or after the call", for 
instance?

It is unfortunate to hear that TLLT are a significant size increase, though not 
entirely surprising - it's a bunch of extra info to encode. I'll be glad to 
have this example to experiment with.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152708

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

[Lldb-commits] [PATCH] D152708: [RFC][Draft] Enable primitive support for Two-Level Line Tables in LLVM

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

Are there any known (or vague/unknown) limitations on the implementation with 
respect to the actual output/on-disk representation? (like, would doing some 
kind of size analysis of the output when using this patch/feature lead to 
incorrect conclusions about the cost of the feature?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152708

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

[Lldb-commits] [PATCH] D153840: [LLDB] Fix buffer overflow problem in DWARFExpression::Evaluate.

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

I'm not sure if this is the right fix - these reads are for implementing 
DW_OP_deref_size, by the looks of it - so I think it does make sense that the 
size read is not the size of the address, but the size specified in the 
DW_OP_deref_size. There is a requirement that DW_OP_deref_size's size may not 
be larger than the system address - so maybe the input that hit this is 
incorrect, and lldb should've failed earlier (validating the size retrieved at 
line 1082 is within the bounds)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153840

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

[Lldb-commits] [PATCH] D153840: [LLDB] Fix buffer overflow problem in DWARFExpression::Evaluate.

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D153840#4474213 , @cmtice wrote:

> Hi Jason,
>
> I had been talking more with David, and yes, I had come to the conclusion 
> that you are both right and that this was not the right fix.  I am planning 
> on reverting this, but I am trying to figure out the right fix to replace it 
> with.  I can't share the source that was causing the bug to manifest, because 
> it's in proprietary code, but David is looking at it and I believe he has 
> come to the conclusion that there is a bug in the DWARF code generation -- we 
> were getting a size of 16, which is absolutely not right.  The question is, 
> in the case of bad DWARF being generated, what (if anything) should the LLDB 
> code here be doing? Should we check the size as soon as we read it in, and 
> assert that  it must be <= 8?  Or something else?  Or just leave the LLDB 
> code entirely alone?
>
> What do you (and other reviewers) think is the right thing to do here?

While it's likely generally under-tested/under-covered, debuggers shouldn't 
crash on invalid/problematic DWARF. So this code should probably abort parsing 
an invalid expression like this - there's probably some codepaths through here 
that do some kind of error handling like the "Failed to dereference pointer 
from" a few lines later. I'd expect a similar error handling should be 
introduced if `size` is invalid (not sure if "invalid" should be `> 
sizeof(lldb::addr_t)` or maybe something more specific (like it should check 
the address size in the DWARF, perhaps - I don't know/recall the /exact/ DWARF 
spec wording about the size limitations)).




Comment at: lldb/source/Expression/DWARFExpression.cpp:1088
 intptr_t ptr;
 ::memcpy(&ptr, src, sizeof(void *));
 // I can't decide whether the size operand should apply to the bytes in

FWIW, I think this code is differently invalid, but figured I'd mention it 
while I'm here - this is probably illegal load widening. If we're processing 
`DW_OP_deref_size(1)` maybe that 1 byte is at the end of a page - reading 
`sizeof(void*)` would read more data than would be valid, and be a problem?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153840

___
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

[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#3082352 , @JDevlieghere 
wrote:

> In D112212#3081935 , @dblaikie 
> wrote:
>
>> 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?
>
> The patch introduces a dependency on Python 3 and unfortunately we still have 
> a small (but important) group of users that haven't fully migrated yet. If 
> the patch were to land before the branch, I'd have to revert it (same result) 
> or find a way to do what `shlex.join` does in Python 2. I did a quick search 
> yesterday and didn't immediately find a good alternative and with the 
> timeline I've given in the past, I also don't think the burden should be on 
> the patch author (Pavel). So that's why I suggested holding off on landing 
> it. If it does turn out to cause a lot of conflicts, I can always reconsider.
>
> But yes, backporting is a real concern, which is the main reason I'm asking 
> not to start making big mechanical changes like replacing all the `six` stuff 
> unless there's a pressing reason to do so.

Ah, fair enough - thanks for the context! (given that it'd be a matter of 
reverting the patch on your release branch - doesn't seem like a huge 
difference, but also easy to wait a few days) - *nod* cleanup's hopefully not 
too expensive to defer for a little while longer, if it's not getting in the 
way of some feature work.


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] D112340: [lldb/Formatters] Remove space from vector type string summaries (NFCI)

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D112340#3081540 , @teemperor wrote:

> In D112340#3081532 , @dblaikie 
> wrote:
>
>> 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?
>
> TestCompactVectors tests this but its unfortunately marked as Darwin-only (as 
> it includes the `Accelerate` framework). Providing a platform-neutral test 
> that just typedef's the same things as Accelerate (in addition to the test 
> including the real framework) seems like a good idea.

Good to know - will keep it in mind, but don't think I'm up for writing that 
test right now myself (not especially familiar with lldb test infrastructure, 
etc).

Was this reported by a buildbot, do you think/know of? Probably would've been 
happy to debug-via-buildbot and fix it that way, but don't recall seeing a 
fail-mail about this (but as always, there's a fair bit of buildbot noise and 
sometimes it's hard to find the cause/issue in a fail-mail such that I end up 
ignoring them)


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] D112340: [lldb/Formatters] Remove space from vector type string summaries (NFCI)

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D112340#3084953 , @teemperor wrote:

> Not actually sure when/how Green Dragon is mailing external people,
> but usually someone just watches the build bot and emails/comments on
> commits that are breaking the buildbot. Green Dragon is more of a
> monte carlo simulation than a real build bot so I wouldn't be
> surprised if it doesn't send automatic failure emails to non-Apple
> people.
>
> Also that 'let's write a test' was more a note for myself to write
> that test (which I hopefully get around today).

Cool - thanks for all the details!


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] D112165: Cleanup a few more PR36048 skips

[David Blaikie via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe6b323379e31: Cleanup a few more PR36048 skips (authored by 
dblaikie).

Repository:
  rG LLVM Github Monorepo

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

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.')
 
-@skipIf(debug_info="gmodules",

[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: lldb/source/API/SBTarget.cpp:1589
 
-  const ConstString csFrom(from), csTo(to);
-  if (!csFrom)
+  llvm::StringRef srFrom(from), srTo(to);
+  if (srFrom.empty())

Aside (since the old code did this too): Generally code should prefer `=` 
initialization over `()` initialization - because the former can't invoke 
explicit conversions, so is easier to read because it limits the possible 
conversion to simpler/safer ones (for instance, with unique ptrs 
"std::unique_ptr p = x();" tells me `x()` returns a unique_ptr or 
equivalent, but `std::unique_ptr p(x());` could be that `x()` returns a 
raw pointer and then I have to wonder if it meant to transfer ownership or not 
- but I don't have to wonder that in the first example because the type system 
checks that for me).

Admittedly StringRef has no explicit ctors - but it's a good general style to 
use imho (would be happy to include this in the LLVM style guide if this seems 
contentious in any way & is worth some discussion/formalization - but I think 
it's just generally good C++ practice & hopefully can be accepted as such)



Comment at: lldb/source/Target/PathMappingList.cpp:35
+  // If we use "path" to construct a FileSpec, it will normalize the path for
+  // us. We then grab the string and turn it back into a ConstString.
+  return FileSpec(path).GetPath();

out of date comment - maybe this comment isn't pulling its weight? The content 
seems fairly simple/probably self explanatory?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112863

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

In D112165#3100608 , @teemperor wrote:

> Small note: gmodules test are never run on Linux, so you actually have to run 
> them on macOS (or I think FreeBSD) to know whether the tests work.

Yeah, I'll admit I didn't test this, but seemed consistent with the other 
changes/cleanup - did this cause any breakage you know of?

Could gmodules be tested on Linux? (I had originally really hoped/tried to 
encourage gmodules to be implemented in a way that'd be compatible with Split 
DWARF but it never quite got there, unfortunately... would've made more overlap 
in functionality/testability/portability, I think)

(I should setup a build environment on my Macbook Pro, but haven't got around 
to it as yet)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112165

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

In D112165#3102055 , @teemperor wrote:

> In D112165#3100929 , @dblaikie 
> wrote:
>
>> In D112165#3100608 , @teemperor 
>> wrote:
>>
>>> Small note: gmodules test are never run on Linux, so you actually have to 
>>> run them on macOS (or I think FreeBSD) to know whether the tests work.
>>
>> Yeah, I'll admit I didn't test this, but seemed consistent with the other 
>> changes/cleanup - did this cause any breakage you know of?
>>
>> Could gmodules be tested on Linux? (I had originally really hoped/tried to 
>> encourage gmodules to be implemented in a way that'd be compatible with 
>> Split DWARF but it never quite got there, unfortunately... would've made 
>> more overlap in functionality/testability/portability, I think)
>>
>> (I should setup a build environment on my Macbook Pro, but haven't got 
>> around to it as yet)
>
> There was just one test on macOS with data-formatter-cpp having defining 
> something that is also in a system header (already fixed that). Also I think 
> watching Green Dragon is enough. There would be a macOS CI if anyone actually 
> cared about Green Dragon being always green.
>
> The tests could in theory be run on Linux just fine, the problem is they just 
> won't test anything. For better or worse (actually, just for the worse), the 
> gmodules testing pretty much relies on re-running all normal API tests on 
> `-fmodules -fcxx-modules -gmodules -fimplicit-module-maps` compiled test 
> sources. But only about 5 of the 1000 API tests even define a module. So the 
> remaining 995 tests just rerun their test as-is unless they include by 
> accident a system header that has a modulemap on the system. No Linux 
> distribution I know comes with modulemaps for its libc so no gmodules 
> functionality will be exercised on the other 995 tests. So gmodules was just 
> disabled as it just made the test suite longer without any benefit. Also I 
> think there were some problems with enabling -fmodules on setups where we had 
> a libc++ module but no modularized libc on the system. I think @labath knows 
> the rationale best.

ooh, yeah - that dose sound like things could benefit from more intentional 
testing - be faster for Apple folks (rather than running the whole test suite 
over again in another config) and portable (so better for everyone)...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112165

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

[Lldb-commits] [PATCH] D113314: [lldb] Use std::string instead of llvm::Twine

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

Alternatively the string concatenation could be inlined, then the Twine usage 
would be correct - but would still need an explicit "str()", like this:

  SendPacketAndWaitForResponse(("QEnableCompression:type:" + avail_name + 
";").str(), response)

Which may or may not be marginally faster because the Twine could cheaply 
compute the total length of the required buffer/presize the string... maybe.


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

https://reviews.llvm.org/D113314

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

[Lldb-commits] [PATCH] D110578: [lldb] Add support for D programming language

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D110578#3123164 , @ljmf00 wrote:

> @dblaikie Maybe you can land this?

Nah, might need someone with more lldb context than me - I don't seem to have a 
clean check-lldb build right now (so don't have a good baseline to validate 
that the patch doesn't regress anything before I commit) & no idea why... :/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110578

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

[Lldb-commits] [PATCH] D113604: [lldb][NFC] Format lldb/include/lldb/Symbol/Type.h

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: lldb/include/lldb/Symbol/Type.h:205-206
+  static bool GetTypeScopeAndBasename(const llvm::StringRef name,
+  llvm::StringRef scope,
+  llvm::StringRef basename,
   lldb::TypeClass &type_class);

Looks like this could've broken some tests - might be best to revert and retest?

The `const StringRef&` should've been changed to `StringRef` per 
@JDevlieghere's comment, but the non-const `StringRef&` were/are probably 
load-bearing - they're the result of calling this function (using mutable ref 
parameters as "out" parameters) & passing them by value breaks that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113604

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

[Lldb-commits] [PATCH] D113604: [lldb][NFC] Format lldb/include/lldb/Symbol/Type.h

[David Blaikie via Phabricator via lldb-commits]

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



Comment at: lldb/include/lldb/Symbol/Type.h:204-206
+  static bool GetTypeScopeAndBasename(const llvm::StringRef name,
+  llvm::StringRef scope,
+  llvm::StringRef basename,





Comment at: lldb/source/Symbol/Type.cpp:665-667
+bool Type::GetTypeScopeAndBasename(const llvm::StringRef name,
+   llvm::StringRef scope,
+   llvm::StringRef basename,




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113604

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

[Lldb-commits] [PATCH] D126668: LLDB: Fix resolving nested template parameters

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

FWIW, clang has support for including template parameter DIEs for template 
declarations (-Xclang -debug-forward-template-params). We could enable that by 
default when tuning for lldb (or maybe we're at the tipping point and we should 
enable it by default in general - I pushed back on that originally when Sony 
folks added/proposed it, under the argument that other debuggers didn't need 
this info - but if they do need the info, so be it)

That feature is also turned on by default in `-gsimple-template-names` which 
we're currently working on adding support to lldb for - so ensuring that lldb 
does the best things when declarations do have template parameters would be 
good. So not filtering based on declaration, but I guess based on "<>" in the 
name and the presenec/absence of template parameter DIEs as the patch does 
currently?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126668

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

[Lldb-commits] [PATCH] D134378: [lldb] Support simplified template names

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.
Herald added a subscriber: JDevlieghere.



Comment at: lldb/packages/Python/lldbsuite/test/make/Makefile.rules:230-231
 
-DEBUG_INFO_FLAG ?= -g
+# DO NOT SUBMIT
+DEBUG_INFO_FLAG ?= -g -gsimple-template-names
 

Oh, nifty place to test this - I'd been testing with the default changed in 
clang itself.



Comment at: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1558-1560
+if (template_param_infos.hasParameterPack()) {
+  args = template_param_infos.packed_args->args;
+}

I think there's some existing problems with lldb here (& gdb, as it happens) - 
this doesn't specify where the parameter pack is in the argument list (looks 
like it assumes it's the only template parameters?) - and doesn't handle the 
possibility of multiple parameter packs in a single parameter list, which I 
think is possible in some corner cases.

I think maybe the existing lldb code can handle some non-pack parameters 
followed by a single pack, but this code assumes it's either non-pack or a 
pack, never both - so might be more restrictive than the existing limitations? 
But maybe not - I might've misread/misremembered the other lldb code.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1564
+  llvm::raw_string_ostream os(template_name);
+  arg.dump(os);
+  if (!template_name.empty()) {

OK, so this line uses Clang's AST printing (avoiding having to reimplement all 
the type printing in lldb, like I've done in llvm-dwarfdump/libDebugInfoDWARF) 
- but I guess there's a reason we can't do that at a higher level for the whole 
template, but have to do it per template argument?

It'd be nice if we could do it for the whole parameter list/template 
specialization, to avoid having to have this <>, etc handling.

I guess it's a circular problem - have to build the name to look up the AST if 
we already have it... so can't build the name /from/ the AST, as such. Fair 
enough.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1585
+  DWARFDIE parent_decl_ctx_die = die.GetParentDeclContextDIE();
+  // TODO: change this to get the correct decl context parent
+  while (parent_decl_ctx_die) {

What's incorrect about it at the moment?

Oh, I see this code has just moved around.



Comment at: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1590-1591
+case DW_TAG_namespace: {
+  const char *namespace_name = parent_decl_ctx_die.GetName();
+  if (namespace_name) {
+qualified_name.insert(0, "::");





Comment at: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1605-1608
+  const char *class_union_struct_name =
+  parent_decl_ctx_die.GetName();
+
+  if (class_union_struct_name) {





Comment at: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1805-1812
+bool is_template = false;
+TypeSystemClang::TemplateParameterInfos template_param_infos;
+if (ParseTemplateParameterInfos(die, template_param_infos)) {
+  is_template = !template_param_infos.args.empty() ||
+template_param_infos.packed_args;
+}
+

Maybe? But the named variable and less indentation's probably good too.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2491-2495
+TypeSP Ty = GetTypeForDIE(die);
+llvm::StringRef qualName = Ty->GetQualifiedName().AsCString();
+auto it = qualName.find('<');
+if (it == llvm::StringRef::npos)
+  return true;

Maybe a comment here? I guess what's happening here is that if the name found 
isn't a template then it isn't relevant/can't match the original query that did 
have template parameters?



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2498-2501
+llvm::StringRef qualNameTemplateParams =
+qualName.slice(it, qualName.size());
+if (templateParams != qualNameTemplateParams)
+  return true;

And this checks the stringified template params match exactly - so there's 
opportunity for improvement in the future to compare with more fuzzy matching 
(I guess we can't use clang's AST because that'd involve making two instances 
of the type somehow which doesn't make a lot of sense) so that users don't have 
to spell the template spec exactly the same way clang does (eg: `x` 
versus `x` - or other more complicated situations with function 
pointers, parentheses, casts, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.l

[Lldb-commits] [PATCH] D134378: [lldb] Support simplified template names

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

Looking pretty good to me FWIW




Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1561
+std::string
+DWARFASTParserClang::GetCPlusPlusQualifiedName(const DWARFDIE &die) {
+  if (!die.IsValid())

Sorry, when I gave feedback on some pieces of this that were just moved around 
rather than new code written in this review - I don't mind the code moving 
around without changes (and optionally before/after the move making 
improvements, but not necessary).

If possible, might simplify the code review to move the code first/separately 
from this change, if the move isn't too meaningless independent of the rest of 
the changes?



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2498-2501
+llvm::StringRef qualNameTemplateParams =
+qualName.slice(it, qualName.size());
+if (templateParams != qualNameTemplateParams)
+  return true;

aeubanks wrote:
> dblaikie wrote:
> > And this checks the stringified template params match exactly - so there's 
> > opportunity for improvement in the future to compare with more fuzzy 
> > matching (I guess we can't use clang's AST because that'd involve making 
> > two instances of the type somehow which doesn't make a lot of sense) so 
> > that users don't have to spell the template spec exactly the same way clang 
> > does (eg: `x` versus `x` - or other more complicated 
> > situations with function pointers, parentheses, casts, etc.
> lldb already requires exact name matching when looking up classes
> 
> e.g.
> ```
> $ cat /tmp/a.cc
> templatestruct Foo {};
> int main() {
> Foo a;
> Foo b;
> Foo c;
> }
> templatestruct Foo {};
> int main() {
> Foo a;
> Foo b;
> Foo c;
> }
> ~/repos/llvm-project/build/cmake$ ./bin/lldb /tmp/a
> (lldb) target create "/tmp/a"
> Current executable set to '/tmp/a' (x86_64).
> (lldb) im loo -t 'Foo< int>'
> (lldb) im loo -t 'Foo'
> 1 match found in /tmp/a:
> id = {0x0058}, name = "Foo", byte-size = 1, decl = a.cc:1, 
> compiler_type = "template<> struct Foo {
> }"
> ```
Yeah, sorry - I understand it requires exact matching currently (because the 
index has the exact string in it) - my comment was forward-looking/idle thought 
that now that with simplified template names we can/have to lookup by base 
name, we have the option to do fuzzier matching when filtering all the base 
name matches - not suggesting that it's a regression that this currently does 
exact matching.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134378

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

[Lldb-commits] [PATCH] D115324: Added the ability to cache the finalized symbol tables subsequent debug sessions to start faster.

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

Been experimenting with this recently and I noticed that loading in the cached 
indexes seems to do a lot of loading - specifically interning a lot of strings 
from the index and the symtab. Does this happen when reading a built-in index 
(apple_names/debug_names) (I don't have an immediately easy way to test this, 
or I Would've before asking)? I'd be surprised if that was the case, which is 
also confusing me as to why it's the case for these cached indexes? I'd have 
expected the cached index to look basically the same as the 
apple_names/debug_names builtin index and have similar performance properties, 
but maybe that's not the case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115324

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

[Lldb-commits] [PATCH] D135979: [lldb][NFCish] Move DWARFDebugInfoEntry::GetQualifiedName() into DWARFASTParserClang

[David Blaikie via Phabricator via lldb-commits]

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

Looked like this was a fairly uncontroversial part of the other patch, so I 
feel confident approving it - but welcome to wait for a second opinion from 
more lldb-affiliated developers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135979

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

[Lldb-commits] [PATCH] D136011: [lldb] Don't check environment default char signedness when creating clang type for "char"

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

I think the place where this will go wrong is in terms of how lldb renders 
`char` values on non-default-char-signedness programs (it'll render them as the 
default-char-signedness, which might be confusing to a user - since they'll be 
looking at literals, etc, that are the other signedness) and how lldb will 
interpret char literals (though that'll already be wrong - since the literals 
are already being parsed with the default-char-signedness, I think).

I'm curious why there were all those expected failures re: PR23069. Were they 
not using the default char signedness? Or is the test using explicit 
signedness, and so whatever platforms happen not to have the explicit value sa 
their default are/were failing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136011

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

[Lldb-commits] [PATCH] D136011: [lldb] Don't check environment default char signedness when creating clang type for "char"

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D136011#3862150 , @labath wrote:

> In D136011#3860637 , @dblaikie 
> wrote:
>
>> I think the place where this will go wrong is in terms of how lldb renders 
>> `char` values on non-default-char-signedness programs (it'll render them as 
>> the default-char-signedness, which might be confusing to a user - since 
>> they'll be looking at literals, etc, that are the other signedness) and how 
>> lldb will interpret char literals (though that'll already be wrong - since 
>> the literals are already being parsed with the default-char-signedness, I 
>> think).
>
> Yes, I'm pretty sure that will happen. OTOH, I don't think there's any value 
> to fix this in a completely satisfactory way. Like, if the whole program was 
> consistently with the non-default signedness, we could try to detect it and 
> then configure the internal AST defaults accordingly. But that's hard to 
> detect, and I'd be surprised if most programs are completely homogeneous like 
> this.
>
> So, overall, I quite like this fix.

Yeah, this line of reasoning (non-homogenaeity) is one I'm on board with, 
thanks for the framing. Basically I/we can think of the debugger's expression 
context as some code that's compiled with the default char signedness always. 
Since char signedness isn't part of the ABI, bits of the program can be built 
with one and bits with the other - and the debugger is just a bit that's built 
with the default.

Works for me. (though lldb's sufficiently out of my wheelhouse I'll leave it to 
others to approve)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136011

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

[Lldb-commits] [PATCH] D136011: [lldb] Don't check environment default char signedness when creating clang type for "char"

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D136011#3870677 , @labath wrote:

> In D136011#3866854 , @aeubanks 
> wrote:
>
>> In D136011#3862150 , @labath wrote:
>>
>>> 
>>
>> Maybe looking for a "char" entry and setting 
>> `ast.getLangOpts().CharIsSigned` from it would probably work in most cases, 
>> but not sure that it's worth the effort. Rendering the wrong signedness for 
>> chars doesn't seem like the worst thing, and this patch improves things (as 
>> the removed `expectedFailureAll` show, plus this helps with some testing of 
>> lldb I've been doing). I can add a TODO if you'd like.
>
> It's not just a question of rendering. This can result in wrong/unexpected 
> results of expressions as well. Something as simple as `p 
> (int)unspecified_char_value` can return a different result depending on 
> whether `unspecified_char_value` is considered signed or unsigned.
> However, I am not convinced that we would be better off trying to detect this 
> would, as that detection can fail as well, only in more unpredictable ways.
>
> In D136011#3868755 , @dblaikie 
> wrote:
>
>> Yeah, this line of reasoning (non-homogenaeity) is one I'm on board with, 
>> thanks for the framing. Basically I/we can think of the debugger's 
>> expression context as some code that's compiled with the default char 
>> signedness always. Since char signedness isn't part of the ABI, bits of the 
>> program can be built with one and bits with the other - and the debugger is 
>> just a bit that's built with the default.
>
> Are you sure about the ABI part? I guess it may depend on how you define the 
> ABI.. It definitely does not affect the calling conventions, but I'm pretty 
> sure it would be an ODR violation if you even had a simple function like `int 
> to_int(char c) { return c; }` in a header, and compiled it with both -fsigned 
> and -funsigned-char. Not that this will stop people from doing it, but the 
> most likely scenario for this is that your linking your application with the 
> C library compiled in a different mode, where it is kinda ok, because the C 
> library does not have many inline functions, and doesn't do much char 
> manipulation.

"ODR violation" gets a bit fuzzy - but yeah, you'd get a different int out of 
that function. My point was since apps would already end up with a mix of 
signed and unsigned most likely, the debugger expression evaluation is one of 
those things mixed one way rather than the other.

But yeah, it's all a bit awkward (& probably will be more for googlers where 
everything's built with the non-default signedness - and now the expression 
evaluator will do the other thing/not that - but hopefully a rather small 
number of users ever notice or care about this). I guess char buffers for 
raw/binary data will be a bit more annoying to look at, dumped as signed 
integers instead of unsigned...

Anyway, yeah, it's all a bit awkward but this seems like the best thing for 
now. Thanks for weighing in!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136011

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

[Lldb-commits] [PATCH] D137098: [lldb] Support simplified template names in the manual index

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D137098#3897289 , @aeubanks wrote:

> updated description with why this doesn't produce false positives with 
> breakpoints
>
> this doesn't support regex function name lookup, not sure if we care enough 
> about the interaction between regexes/function names under simple template 
> names. if we do, we could instead add template parameters to the names we put 
> in the manual index. I did take a quick look at doing that but it'd require 
> more work

Presumably that'd then lead to divergence between manual and automatic index - 
which would be bad. So if this behavior is the same between automatic and 
manual index with simplified template names, that's probably good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137098

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

[Lldb-commits] [PATCH] D137583: [lldb] Fix simple template names and template params with scope qualifiers

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a subscriber: aaron.ballman.
dblaikie added a comment.

@aaron.ballman does this seem OK? (this was based on my suggestion in the 
related review linked in the description)

It probably needs tests in clang too - not sure if there's an opportunity to 
use a unit test to simplify access to the printing policy & exercise this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137583

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

[Lldb-commits] [PATCH] D137583: [lldb] Fix simple template names and template params with scope qualifiers

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D137583#3917706 , @aaron.ballman 
wrote:

>> ...we expect template params to be fully qualified when comparing them for 
>> simple template names
>
> So lldb is not inspecting the AST, they're doing reflection (of a sort) on 
> the pretty printed names? Or am I misunderstanding something?

Not reflection as such - but building names for the user, but partly from the 
AST - basically LLDB wants to be able to produce the same name that CGDebugInfo 
produces - so, maybe it should produce it the same way as CGDebugInfo, which 
isn't to use the pretty printer from scratch.

@aeubanks would this work for lldb's use case? 
https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGDebugInfo.cpp#L5229
 it'd be identical to the original debug info generation, and looks like it 
doesn't require a printing policy change/feature. Sorry I didn't think of that 
earlier. I guess since `Qualified` would be `false` for lldb's use case, you 
could go down into the implementation and just call the unqualified side 
directly: `NamedDecl::printName(OS, Policy);` should print it unqualified for 
this name, but respect the qualified printing policy flag for any nested names, 
parameters, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137583

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

[Lldb-commits] [PATCH] D137983: [lldb] Disable looking at pointee types to find synthetic value for non-ObjC

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: lldb/source/Core/ValueObject.cpp:2676-2677
 if (!m_deref_valobj) {
-  if (HasSyntheticValue()) {
+  // FIXME: C++ stdlib formatters break with incomplete types (e.g.
+  // `std::vector &`). Remove ObjC restriction once that's resolved.
+  if (Language::LanguageIsObjC(GetPreferredDisplayLanguage()) &&

Maybe worth filing a bug and referencing it here?

Is this limitation still necessary if the incomplete type has template 
parameter DIEs? (I guess probably yes, because it'll be missing member 
descriptions, etc)

& does this path get hit if the type is declared in one CU but defined in 
another? (& does the inf recurse/crash loop still get hit in that case, without 
this patch?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137983

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

[Lldb-commits] [PATCH] D137583: [lldb] Fix simple template names and template params with scope qualifiers

[David Blaikie via Phabricator via lldb-commits]

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

Awesome!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137583

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

[Lldb-commits] [PATCH] D138539: Use std::nullopt_t instead of NoneType (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.

Sounds good to me, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138539

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

[Lldb-commits] [PATCH] D138834: [lldb] Fix simple template names interaction with debug info declarations

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:747-749
+  if (ParseTemplateParameterInfos(die, template_param_infos) &&
+  (!template_param_infos.args.empty() ||
+   template_param_infos.packed_args)) {

Could invert this condition and use an early `return ConstString();` to reduce 
indentation - but I guess this looks more similar to the definition handling 
case and so might be worth keeping in the same shape. (maybe 
`ParseTemplateParameterInfos` should return true only if the result is !empty 
or has packed args, so that test doesn't need to be repeated at multiple 
callers?)



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3069-3079
+if (auto i = test_base_name.find('<'); i != llvm::StringRef::npos) {
+  llvm::StringRef test_template_params =
+  test_base_name.slice(i, test_base_name.size());
+  // Bail if template parameters don't match.
+  if (test_template_params != template_params.GetStringRef())
+return true;
+} else {

might be worth flipping to reduce indentation:
```
auto i = test_base_name.find('<');
if (i == llvm::StringRef::npos)
  return true;
llvm::StringRef test_template_params = ...
if (test_template_params != template_params.GetStringRef())
  return true;
```



Comment at: lldb/test/API/lang/cpp/unique-types3/main.cpp:1-14
+#include 
+#include 
+
+std::atomic a1;
+std::atomic a2;
+std::atomic a3;
+

maybe good to simplify this a bit - rather than using big/complex templates 
like std::atomic and std::vector, instead using test-only, simpler templates to 
keep the test a bit more focussed (less likely to fail for unrelated 
reasons/other regressions)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138834

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

[Lldb-commits] [PATCH] D138834: [lldb] Fix simple template names interaction with debug info declarations

[David Blaikie via Phabricator via lldb-commits]

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

I think this is good - welcome to wait for a second opinion if you prefer, or 
folks can provide feedback post-commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138834

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

[Lldb-commits] [PATCH] D138939: [WIP][clang] adds a way to provide user-oriented reasons

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

Sorry if this is derailing, but I wonder/worry about a few things here:

1. Compounding structured output with phraseology - it seems like it might be 
worthwhile for these to be separate issues (doesn't mean the terminal output 
has to say exactly the same thing - as you've mentioned, we could have some 
fields that aren't rendered in some modes (eg: maybe a terminal only gets the 
summary, and not the reason - or perhaps you can ask for reasons to be provided 
if you don't mind the verbosity)). In the example case include here - 
describing the template parameter kind mismatch seems OK for Clang's current 
textual diagnostic experience & I don't think that would need to be limited to 
only the SARIF output?
2. Could the new phraseology be included in a separate/parallel diagnostic 
file, essentially akin to a translation? Clang never did get support for 
multiple languages in its diagnostics, but maybe this is the time to do that 
work? And then have this new phrasing as the first "translation" of sorts?
3. I'm not sure I'd commit to calling the current diagnostic experience 
"legacy" just yet & maybe this is my broadest feedback: It'd be great to avoid 
a two-system situation with the intent to merge things back in later.

I think what I'd like to see (though I'm far from the decider in any of this) 
would be that the existing underlying structured diagnostics system could have 
a new emission system, that produces SARIF instead of text on a terminal - same 
underlying structured representation, but providing that more directly (in 
SARIF/JSON/etc) so that it's not lossy in the conversion to an output format - 
essentially a machine-readable mode for clang's diagnostic output. (to a file, 
to the terminal, beside the existing terminal output or instead of it, whatever 
folks prefer) - this would, ideally, initially require no changes to diagnostic 
API usage across clang.
Diagnostic quality improvements, like the template parameter kind mismatch can 
be committed independently as desired.
Diagnostic infrastructure could be improved/expanded - adding the "reason" 
field, and a way to emit that (perhaps only in SARIF output, but maybe there's 
room for a flag to emit it in terminal diagnostics too).
Multiple diagnostic text files supported, and the ability to choose which 
diagnostic text - initially with the intent to support this newly proposed 
phrasing approach, but equally could be used for whole language translation.

Pretty much all of these features could be implemented somewhat in parallel, 
each feature would be of value independently to varying degrees, and we 
wouldn't end up with a deprecated/legacy/not ready yet situation (the only "not 
ready yet" part might be adding the new diagnostic phrasings - perhaps 
initially there'd be some way to fallback to the traditional diagnostics, so 
that the whole database didn't need to be translated wholesale - and once it's 
all translated and there's a lot of user feedback on the direction, we could 
consider changing the diagnostic database default, perhaps eventually 
deprecating the old database, and eventually removing it - without major API 
upheaval/renaming/addition/removal)




Comment at: clang/lib/Basic/DiagnosticIDs.cpp:524
+
+// FIXME(spaceship): default when C++20 becomes the default
+bool operator==(const DiagDesc &Other) const {

I don't think LLVM generally uses `(xxx)` in FIXMEs, or if we do it's probably 
only for bug numbers and maybe developer user names (though the latter's not 
ideal - people come and go, bug numbers are forever (kinda)) - not sure what 
the "spaceship" in there is meant to communicate (I'm aware of the C++20 
spaceship operator, but I'd expect the thing in the `()` to be some list of 
known entities that we keep track of somewhere?)

Maybe `// FIXME: Use spaceship operator in C++20`?



Comment at: clang/lib/Basic/DiagnosticIDs.cpp:812-825
 namespace {
-  struct WarningOption {
-uint16_t NameOffset;
-uint16_t Members;
-uint16_t SubGroups;
-StringRef Documentation;
-
-// String is stored with a pascal-style length byte.
-StringRef getName() const {
-  return StringRef(DiagGroupNames + NameOffset + 1,
-   DiagGroupNames[NameOffset]);
-}
-  };
-}
+struct WarningOption {
+  uint16_t NameOffset;
+  uint16_t Members;
+  uint16_t SubGroups;
+  StringRef Documentation;
+

Unrelated/accidental reformatting? (please commit separately if it's being 
committed)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138939

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

[Lldb-commits] [PATCH] D102942: Remove or use variables which are unused but set.

[David Blaikie via Phabricator via lldb-commits]

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

Sure, all sounds good - if you can, please reach out to the authors of any of 
the semantics changing changes (the ones related to the `Changed` values in 
transformations) to see if they could add missing test coverage.




Comment at: llvm/lib/Target/AArch64/GISel/AArch64PostSelectOptimize.cpp:183
 Changed |= optimizeNZCVDefs(BB);
-  return true;
+  return Changed;
 }

Might want to CC/respond to whoever wrote this to see if someone missed test 
coverage for this code.



Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1247-1250
   if (LoLoop.Preheader)
 LoLoop.Start = SearchForStart(LoLoop.Preheader);
   else
+return Changed;

Maybe in a separate patch would be good to switch this around to:
```
if (!LoLoop.Preheader)
  return Changed;
LoLoop.Start = SearchForStart(LoLoop.Preheader);
```



Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1250
   else
-return false;
+return Changed;
 

Also might be worth reaching out to authors to check that this change is 
intended & possibly tested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102942

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

[Lldb-commits] [PATCH] D102942: Remove or use variables which are unused but set.

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D102942#2791204 , @mbenfield wrote:

> I also heard via email from Amara Emerson that the change is fine for similar 
> reasons.

Great, thanks for checking!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102942

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

[Lldb-commits] [PATCH] D101921: [MC] Make it possible for targets to define their own MCObjectFileInfo

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D101921#2786245 , @MaskRay wrote:

> Because of `new MCObjectFileInfo`, we cannot use a forward declaration 
> (incomplete class) to replace `#include "llvm/MC/MCObjectFileInfo.h"` in 
> `TargetRegistry.h`.
>
> I thought about moving `TargetRegistry.{h,cpp}` from Support to Target. 
> However, it doesn't work because Bitcode/Object call 
> `TargetRegistry::lookupTarget` and Bitcode/Object are lower than Target.
>
> @compnerd @dblaikie @mehdi_amini Do you have suggestions on fixing the 
> layering?

Looks like a big patch and a bit hard for me to page in all the context. Could 
you summarize the layering constraints/what headers/types/functions are in 
which libraries and why they are there/what constrains them?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101921

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

[Lldb-commits] [PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D100581#2793775 , @ldionne wrote:

> Hello! There are still some false positives, for example this one is breaking 
> the libc++ CI: 
> https://buildkite.com/llvm-project/libcxx-ci/builds/3530#8679608a-200b-4a48-beb4-a9e9924a8848
>
> Would it be possible to either fix this quickly or revert the change until 
> the issue has been fixed? Our pre-commit CI is going to be stalled until this 
> is fixed. Thanks!

Looks like a true positive in libc++ ( 
https://github.com/llvm/llvm-project/blob/main/libcxx/include/regex ) - the 
"__j" variable is initialized, then incremented, but never read (except to 
increment). Is that a bug in libc++? Was __j meant to be used for something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100581

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

[Lldb-commits] [PATCH] D103842: NFC: .clang-tidy: Inherit configs from parents to improve maintainability

[David Blaikie via Phabricator via lldb-commits]

dblaikie created this revision.
dblaikie added reviewers: hokein, aaron.ballman, jyknight, mehdi_amini, kuhnel.
Herald added subscribers: dcaballe, cota, teijeong, rdzhabarov, tatianashp, 
msifontes, jurahul, Kayjukh, grosul1, Joonsoo, liufengdb, aartbik, lucyrfox, 
mgester, arpith-jacob, antiagainst, shauheen, rriddle.
dblaikie requested review of this revision.
Herald added subscribers: llvm-commits, lldb-commits, Sanitizers, cfe-commits, 
stephenneuendorffer, nicolasvasilache, aheejin.
Herald added projects: clang, Sanitizers, LLDB, MLIR, LLVM.

In the interests of disabling misc-no-recursion across LLVM (this seems
like a stylistic choice that is not consistent with LLVM's
style/development approach) this NFC preliminary change adjusts all the
.clang-tidy files to inherit from their parents as much as possible.

This change specifically preserves all the quirks of the current configs
in order to make it easier to review as NFC.

I validatad the change is NFC as follows:

for X in `cat ../files.txt`;
do

  mkdir -p ../tmp/$(dirname $X)
  touch $(dirname $X)/blaikie.cpp
  clang-tidy -dump-config $(dirname $X)/blaikie.cpp > ../tmp/$(dirname $X)/after
  rm $(dirname $X)/blaikie.cpp

done

(similarly for the "before" state, without this patch applied)

for X in `cat ../files.txt`;
do

  echo $X
  diff \
../tmp/$(dirname $X)/before \
<(cat ../tmp/$(dirname $X)/after \
  | sed -e 
"s/,readability-identifier-naming\(.*\),-readability-identifier-naming/\1/" \
  | sed -e "s/,-llvm-include-order\(.*\),llvm-include-order/\1/" \
  | sed -e "s/,-misc-no-recursion\(.*\),misc-no-recursion/\1/" \
  | sed -e "s/,-clang-diagnostic-\*\(.*\),clang-diagnostic-\*/\1/")

done

(using sed to strip some add/remove pairs to reduce the diff and make it easier 
to read)

The resulting report is:

  .clang-tidy
  clang/.clang-tidy
  2c2
  < Checks:  
'clang-diagnostic-*,clang-analyzer-*,-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-readability-identifier-naming,-misc-no-recursion'
  ---
  > Checks:  
'clang-diagnostic-*,clang-analyzer-*,-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-misc-no-recursion'
  compiler-rt/.clang-tidy
  2c2
  < Checks:  
'clang-diagnostic-*,clang-analyzer-*,-*,clang-diagnostic-*,llvm-*,-llvm-header-guard,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes'
  ---
  > Checks:  
'clang-diagnostic-*,clang-analyzer-*,-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-llvm-header-guard'
  flang/.clang-tidy
  2c2
  < Checks:  
'clang-diagnostic-*,clang-analyzer-*,-*,llvm-*,-llvm-include-order,misc-*,-misc-no-recursion,-misc-unused-parameters,-misc-non-private-member-variables-in-classes'
  ---
  > Checks:  
'clang-diagnostic-*,clang-analyzer-*,-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-llvm-include-order,-misc-no-recursion'
  flang/include/flang/Lower/.clang-tidy
  flang/include/flang/Optimizer/.clang-tidy
  flang/lib/Lower/.clang-tidy
  flang/lib/Optimizer/.clang-tidy
  lld/.clang-tidy
  lldb/.clang-tidy
  llvm/tools/split-file/.clang-tidy
  mlir/.clang-tidy

The `clang/.clang-tidy` change is a no-op, disabling an option that was never 
enabled.
The compiler-rt and flang changes are no-op reorderings of the same flags.

(side note, the .clang-tidy file in parallel-libs is broken and crashes
clang-tidy because it uses "lowerCase" as the style instead of "lower_case" -
so I'll deal with that separately)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D103842

Files:
  clang/.clang-tidy
  compiler-rt/.clang-tidy
  flang/.clang-tidy
  flang/include/flang/Lower/.clang-tidy
  flang/include/flang/Optimizer/.clang-tidy
  flang/lib/Lower/.clang-tidy
  flang/lib/Optimizer/.clang-tidy
  lld/.clang-tidy
  lldb/.clang-tidy
  llvm/.clang-tidy
  llvm/tools/split-file/.clang-tidy
  mlir/.clang-tidy

Index: mlir/.clang-tidy
===
--- mlir/.clang-tidy
+++ mlir/.clang-tidy
@@ -1,19 +1,8 @@
-# Almost identical to the top-level .clang-tidy, except that {Member,Parameter,Variable}Case use camelBack.
-Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,readability-identifier-naming'
+InheritParentConfig: true
 CheckOptions:
-  - key: readability-identifier-naming.ClassCase
-value:   CamelCase
-  - key: readability-identifier-naming.EnumCase
-value:   CamelCase
-  - key: readability-identifier-naming.FunctionCase
-value:   camelBack
   - key: readability-identifier-naming.MemberCase
 value:   camelBack
   - key: readability-identifier-naming.ParameterCase
 value:  

[Lldb-commits] [PATCH] D103842: NFC: .clang-tidy: Inherit configs from parents to improve maintainability

[David Blaikie via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc5d56fec502f: NFC: .clang-tidy: Inherit configs from parents 
to improve maintainability (authored by dblaikie).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103842

Files:
  clang/.clang-tidy
  compiler-rt/.clang-tidy
  flang/.clang-tidy
  flang/include/flang/Lower/.clang-tidy
  flang/include/flang/Optimizer/.clang-tidy
  flang/lib/Lower/.clang-tidy
  flang/lib/Optimizer/.clang-tidy
  lld/.clang-tidy
  lldb/.clang-tidy
  llvm/.clang-tidy
  llvm/tools/split-file/.clang-tidy
  mlir/.clang-tidy

Index: mlir/.clang-tidy
===
--- mlir/.clang-tidy
+++ mlir/.clang-tidy
@@ -1,19 +1,8 @@
-# Almost identical to the top-level .clang-tidy, except that {Member,Parameter,Variable}Case use camelBack.
-Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,readability-identifier-naming'
+InheritParentConfig: true
 CheckOptions:
-  - key: readability-identifier-naming.ClassCase
-value:   CamelCase
-  - key: readability-identifier-naming.EnumCase
-value:   CamelCase
-  - key: readability-identifier-naming.FunctionCase
-value:   camelBack
   - key: readability-identifier-naming.MemberCase
 value:   camelBack
   - key: readability-identifier-naming.ParameterCase
 value:   camelBack
-  - key: readability-identifier-naming.UnionCase
-value:   CamelCase
   - key: readability-identifier-naming.VariableCase
 value:   camelBack
-  - key: readability-identifier-naming.IgnoreMainLikeFunctions
-value:   1
Index: llvm/tools/split-file/.clang-tidy
===
--- llvm/tools/split-file/.clang-tidy
+++ llvm/tools/split-file/.clang-tidy
@@ -1,19 +1,8 @@
-# Almost identical to the top-level .clang-tidy, except that {Member,Parameter,Variable}Case use camelBack.
-Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,readability-identifier-naming'
+InheritParentConfig: true
 CheckOptions:
-  - key: readability-identifier-naming.ClassCase
-value:   CamelCase
-  - key: readability-identifier-naming.EnumCase
-value:   CamelCase
-  - key: readability-identifier-naming.FunctionCase
-value:   camelBack
   - key: readability-identifier-naming.MemberCase
 value:   camelBack
   - key: readability-identifier-naming.ParameterCase
 value:   camelBack
-  - key: readability-identifier-naming.UnionCase
-value:   CamelCase
   - key: readability-identifier-naming.VariableCase
 value:   camelBack
-  - key: readability-identifier-naming.IgnoreMainLikeFunctions
-value:   1
Index: llvm/.clang-tidy
===
--- llvm/.clang-tidy
+++ llvm/.clang-tidy
@@ -1,19 +1 @@
-Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,readability-identifier-naming'
-CheckOptions:
-  - key: readability-identifier-naming.ClassCase
-value:   CamelCase
-  - key: readability-identifier-naming.EnumCase
-value:   CamelCase
-  - key: readability-identifier-naming.FunctionCase
-value:   camelBack
-  - key: readability-identifier-naming.MemberCase
-value:   CamelCase
-  - key: readability-identifier-naming.ParameterCase
-value:   CamelCase
-  - key: readability-identifier-naming.UnionCase
-value:   CamelCase
-  - key: readability-identifier-naming.VariableCase
-value:   CamelCase
-  - key: readability-identifier-naming.IgnoreMainLikeFunctions
-value:   1
-
+InheritParentConfig: true
Index: lldb/.clang-tidy
===
--- lldb/.clang-tidy
+++ lldb/.clang-tidy
@@ -1,2 +1,2 @@
-# Checks enabled in the top-level .clang-tidy minus readability-identifier-naming
-Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes'
+Checks: '-readability-identifier-naming'
+InheritParentConfig: true
Index: lld/.clang-tidy
===
--- lld/.clang-tidy
+++ lld/.clang-tidy
@@ -1,19 +1,8 @@
-# Almost identical to the top-level .clang-tidy, except that {Member,Parameter,Variable}Case use camelBack.
-Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-mis

[Lldb-commits] [PATCH] D104380: [lldb] Set return object failed status even if error string is empty

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

This might be a case of an overly reduced patch - this patch alone I'd expect 
to be observable in some way, and tested. If the "instring.empty()" condition 
never fires, then I'd expect to change that to an assert, rather than changing 
the semantics of it in an unobservable way.

But I see there were follow-up NFC/refactoring commits that did take advantage 
of the new behavior - might've been more suitable to group the refactoring with 
this change in behavior, at least in one example of each of the 3 changes here 
(eg: for each one, change one caller and the implementation - then change the 
rest of the callers in separate follow-up commits if you prefer to break them 
out that way, to keep patches small in case they do introduce any regressions)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104380

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