Ooh, nice catch. This must have been fun to debug.

LGTM. (BTW, python 3.1's EOL was 6 years ago)

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

In D52139#1243170, @teemperor wrote:

Anyway, the actual issue is related Python 3: Arch Linux (and probably
some other distributions that "already" upgraded to Python 3 as
default) will launch Python 3 when the test script calls python ....
And for some reason in Python 3.7, we will not inherit our FD from our
pipe to the subprocess, which causes this strange behavior when write
to the unrelated FD number in ConnectToRemote. When I explicitly call
Python 2.7 from the test, everything runs as expected.

The python docs don't see to mention this change (and it seems like a
bug to me), so I'm open for ideas how to fix this properly. In any
case this problem doesn't block this review.

In D48393#1243195, @JDevlieghere wrote:

In D48393#1139327, @labath wrote:

The only sane algorithm I can come up right now is to make the list of parsed dies local to each thread/parsing entity (e.g. via a "visited" list), and only update the global map once the parsing has completed (successfully or not). This can potentially duplicate some effort where one thread parses a type only to find out that it has already been parsed, but hopefully that is not going to be the common case. The alternative is some complicated resource cycle detection scheme.

I gave this a shot in D52406 but I'm afraid it's too simple to be correct. Pavel, could you give it a look and let me know whether that was what you had in mind?

I believe you should be able to use lldb-test to write this kind of a test. I am thinking of a sequence like:

yaml2obj %p/Inputs/test.yaml > %t/test.yaml
yaml2obj %p/Inputs/test.o.yaml > %t/test.o.yaml
lldb-test breakpoint %t/test.yaml %s | FileCheck %s
b main
CHECK-LABEL: b main
CHECK: Address: {{.*}}main + 13 at test.cpp:2:3

I am not sure which thread it was, but I think I mentioned somewhere that lldb-test would be better suited to write this kind of a test. bin/lldb-test object-file -contents should print the contents of the sections, which should reflect the relocations that have been applied to them. Then you could write the test as:
yaml2obj | lldb-test object-file -contents | FileCheck

This makes sense to me, but the windows folks should say what is best for their platform.

Add the file to module map.

I think the issue here is that on windows we don't have the ability(*) to pipe stdio through lldb, so if you create a process without a console, you will not be able to use see it's output or pass it some input (probably fine for gui apps, but bad for console ones). This means your approach will also make things inconsistent with other platforms, only in a different way. (I don't really have an opinion which one is better/worse.)

lgtm modulo typos

In D51576#1223596, @clayborg wrote:

We want to ensure that both Apple and DWARF5 tables never get generated though. That would waste a lot of space. I would say if DWARF5 tables are enabled, then we need ensure we disable Apple tables.

In D51576#1223234, @aprantl wrote:

This is DWARF5+ only, right? (We shouldn't change the preference of Apple accelerator tables for DWARF 4 and earlier).

looks good to me

As for the original patch, which we've kind of hijacked, I think it is a good idea to commit this for now.

In D51208#1214743, @dblaikie wrote:

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:
real 0m5.554s
user 0m43.764s
sys 0m6.748s

setting a breakpoint on a non-existing function with accelerator tables:
real 0m3.517s
user 0m3.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?

I've been wanting to implement something like this for a long time, so thank you for working on this. :)

Also, to answer Zachary's question:

Not blocked on anything, just haven't gotten around to committing it yet. Feel free to land it for me.

You are correct that we were being too strict in our requirements here. Though if these structs do have linkage names they could actually be useful (eg as a substitute for the "qualified name hash" in lldb) and we could consider adding them as an extension.

In D51208#1212950, @dblaikie wrote:

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

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.

This is another one of classes we should probably get rid of (it looks like all of this functionality is available in StringRef), but while we have it, it might as well be tested.

Thank you for writing the test. This is more-or-less what I had in mind. Just make sure we don't put the test files in /tmp (e.g., because windows doesn't have it).

I am not sure my LGTM is enough, but in any case, it looks good to me.

In D50599#1197819, @erik.pilkington wrote:

In D50599#1197781, @labath wrote:

That's an interesting idea. The slight hickup I see is that for my use case, I only need to update C1->C2 for the top-level function (and not, e.g. for the C1 instances nested inside local names). This kind of thing is easy to do on the AST, but not from within the parseCtorDtor() function. I suspect that may be true for other transformations as well (I'm not sure if there is a good spec of what should exactly be substituted by D50586, but I think this could be an issue there too). I suppose that could be made to work by overriding parseLocalName (and anything else that may embed a function encoding), but it won't be ideal, because we would need to duplicate some of the parsing logic:

disableC1substitution();
Node *Encoding = parseEncoding();
enableC1substitution();
if (consumeIf('s'))
  // do stuff
 
if (consumeIf('d'))
  // do other stuff
 
Node *Entity = parseName(State);
...

Or we could just not support substitution in local names (I think LLDB has bigger problems when working with local objects than missing constructors).

Couldn't you just track the <encoding> depth by overriding parseEncoding, and only do the analysis in parseCtorDtor for top-level encodings?

What would you say to moving this class to the header, so that it can be used in all the other byte-counting patches too?

cool

It seems to me this will not handle multi-error objects correctly, but I don't know much about your use case to know whether that is an issue or not.

In D50599#1197282, @erik.pilkington wrote:

This is really cool, thanks for putting this together! In order to properly recreate the mangled name, we would have to track a far amount more information in the AST, looks like you have a pretty good start. I think that the set of transformations that we could support here would be pretty limited. For instance, substituting int for int* in a mangled name is probably a non-starter because we would have to add a substitution, which means that we would have to track substitutions from the AST (in order to tell when int* already appeared in the mangled name and should be remangled as a substitution), and adjust future substitutions numbers, which sounds like it would be hard to get right. Just replacing one <builtin-type> for another should be fine though (since those can't be substitutions), as should the constructor thing you mentioned.

In D50586#1197041, @sgraenitz wrote:

Doing things this way, would require adding more hooks to the demangler, which does not seem particularly nice. OTOH, exposing the AST would allow other interesting uses too. ...

Yes, I am totally with you. Doing this on an exposed AST adds lots of potential, but it also seems like a bigger functional change to me, which requires proper planning and feature discussions. Thus, I am in favour of doing things step by step here. I'd first mimic the old LLDB behaviour with the new demangler and remove FastDemangle from LLDB. Afterwards we can plan and discuss at which places and for which extended functionality we can make use of an exposed AST.

In the meantime, I think llvm::itaniumFindTypesInMangledName() even adds value on its own as it's a simple way to visit parameter types in a mangled name.

What do you think?

I think that's up to Erik mostly. Doing things this way definitely makes LLDB's life easier, but OTOH, I am generally trying to avoid cleaning up LLDB by pushing the mess into LLVM. This is not a particularly large mess, but it still a very odd specialized api that will only ever be used by a single customer.

I've recently ran into another case where I've needed to modify a mangled name.

In D49739#1194037, @apolyakov wrote:

I think those options don't fit. I'm looking for behavior like this:

~/workspace/gsoc/build/bin/lldb-server gdbserver --pipe 1 localhost:0
36251

Here lldb-server prints out the port number it is listening on and continues running.

If debugserver can choose a tcp port in case of passed hostname:0 then probably all we need to do is to add analogue of --pipe option.

To end things on a positive note: I think the test coverage is pretty good (I'm sure somebody will suggest switching to lit), and I think a VS code plugin would be a great addition to lldb.

I think this looks great. I am very happy with the final result. Thank you for your patience, and thanks again for taking on this task.

I am not sure I'll have the resources to see this review through, so I'd prefer to leave this to someone else.

In D50213#1191664, @dblaikie wrote:

Actually - potentially we could merge the accelerator table options into this too, to promote them to an IR feature as well. But it looks like they aren't really supported on a per-CU basis, but are across the whole compilation only. (read the spec of debug_names - I see, best to emit the widest entry possible (ie: bad idea to emit one table per CU if we're emitting multiple CUs in a single object file (LTO) - and no need not to emit it if it's been requested, I suppose - though I guess a user might reasonably request no accelerator table for /some/ CUs they expect not to debug much/are OK with the debugger indexing slowly later on if they do)). But Apple's names don't support multiple CUs at all, so they can't be unified there anyway.

Updating the module map too (thanks).

In D50198#1187653, @stella.stamenova wrote:

In D50198#1186890, @labath wrote:

BTW, since we switched to the lit test runner, the expectedFlakey decorators don't actually do anything.

Oh, haha, that explains why it made no difference. Should we remove the expectedFlakey decorators entirely, so that they don't confuse anyone else (and replace them with a comment) or is there a plan to make them work again in the future?