It's not exactly what I had in mind, but I kinda like it :)

yay

In D144392#4188940, @kpdev42 wrote:

Gdb does this for example:

GDB optimizes for stepping the mainline code. If a signal that has handle nostop and handle pass set arrives while a stepping command (e.g., stepi, step, next) is in progress, GDB lets the signal handler run and then resumes stepping the mainline code once the signal handler returns. In other words, GDB steps over the signal handler. This prevents signals that you’ve specified as not interesting (with handle nostop) from changing the focus of debugging unexpectedly.

(https://sourceware.org/gdb/onlinedocs/gdb/Signals.html), which seems reasonable.

In D145487#4174832, @Michael137 wrote:

• Remove virtual function pointer test for now

You may want to check that pointers to virtual functions get printed here. In particular, that 0x00000000000000010000000000000000 (pointer to the first virtual function) does not get printed as nullptr or something like that (due to the truncation of the 128-bit value to zero).

I kinda like this.

In D144181#4142295, @Michael137 wrote:

We thought a bit about what it would take to link a constructor declaration DIE to the various definitions (e.g., via a DW_AT_LLVM_complete_ctor_linkage_name or DW_AT_LLVM_complete_ctor_ref). The issue with this is that it would mess with type uniquing. E.g., what if two different CUs each had a constructor declaration but differed only in which definitions they linked to? Even if dsymutil were to be able to merge the declarations into a single one, LLDB would still have to support the case where two constructor DIEs for the same type point to different definitions depending on how they were used in the corresponding object files.

One could instead have some new attribute on the various constructor definitions specifying which constructor type it is, and then implement @pavel's suggestion of attaching multiple asm labels to the CXXConstructorDecl. To support that in LLDB we'd have to do a lookup in the DWARF index and filter appropriately.

In D144181#4140831, @Michael137 wrote:

The construction of the mangled name does not require getting just the abi tags of the constructor itself right. Rather, it requires knowing the abi tag annotations of all arguments (template and regular) of the constructor, and template arguments of all enclosing classes. Is the information in this patch enough to reconstruct this name (_ZN2S1I1AB4asdfE2S2I1BB4asdfEC2I1CB4asdfEET_1DB4asdf, a.k.a, S1<A[abi:asdf]>::S2<B[abi:asdf]>::S2<C[abi:asdf]>(C[abi:asdf], D[abi:asdf]))?

That's a fair point. I purposefully didn't support abi_tags on all possible declarations because that would've ballooned the debug-info size too much. And we get away with it because libc++ exclusively tags functions. But of course there's no guarantee that they won't start tagging structures or namespaces

In D144181#4139446, @Michael137 wrote:

Hmm, I'd sort of still be inclined to look further at the linkage name option - but maybe the existence of a situation where the usage is built with debug info, but the out of line ctor definitions are all in another library without debug info is a sufficiently motivating use case to justify the addition of these attributes to the ctor declaration. *shrug* Carry on then.

That would've been convenient but I don't think we can get it right with attaching the linkage name (even if we are willing to do the search for the correct linkage name). That would essentially coalesce the possibly multiple definitions of the constructor into a single one (since there's only ever a single CXXConstructorDecl in the AST for a given constructor in the source)

(CC @labath)

Nice catch.

In D142926#4124297, @bulbazord wrote:

In D142926#4123492, @labath wrote:

Well... right now, there isn't anything else to do there.

Anyway, I don't think this is particularly important. I don't know even if the distros would like to do this or whether to they'd prefer to ship unchanged headers. However, to me, it seems like the choice of the distribution method (framework vs. "traditional" unix layout) should be orthogonal to the choice of deswig-ifying the headers.

I think I agree with you about the choice of distribution method being orthogonal to choice of removing the swig ifdefs from the headers. That being said, I don't know if any other distributors would care for this and I'm not sure who I would ask. I added support for using unifdef in the LLDB.framework case because I can ensure it is used and tested. I am hesitant to add support without somebody to test/use this.

Sort of tangential to this, the only other target where I think this would be used would be the lldb-headers target. That seems to install all the lldb headers, including all the private lldb headers. I could use unifdef there (assuming it's available). Is it intended that it installs all the private lldb headers though?

I'm afraid you're fixing this at the wrong end. This kind of complex thread control does not belong inside the debug stub.

There's one thing that's not clear to me about this patch. What is the relationship between qGetTLSAddr, as implemented here, and its implementation in GDB? I guess they're not compatible since we're not sending or using the offset and link map fields, but it's not clear to me whether we're at least implementing some subset of gdb functionality (e.g., what would gdbserver do if it received one of the packets that we're sending here). If we're not, then I don't think we should be using the same packet for this purpose.

In D142926#4103185, @bulbazord wrote:

In D142926#4102287, @labath wrote:

I like this.

I'm not sure what it would take, but I think it'd be nice if the de-swig-ification was not specific to the framework build. Ideally, I'd make it controlled by a separate cmake variable, (autodetected to on if the relevant tool is found, but also overridable by the user).

Yeah we could search for unified earlier in the process and use it when we distribute headers. The de-swig-ification of the headers is primarily for distribution, do any other platforms distribute the headers?

Thank you for your patience. I'm really happy with the overall result here.

I like this.

In D142309#4075745, @JDevlieghere wrote:

LGTM, but I wonder when we'll be able to get rid of RenderScript completely (CC @labath)

In D138618#4094933, @clayborg wrote:

• My main source of frustration was that my concern is getting overlooked/ignored (not necessarily your fault -- I've been told I am not always sufficiently clear). I think that is something we could live with, if we thing the other cleanups in this patch are worth it (which could very well be the case) -- however, I would want us to be clear that's what we're doing.

I do want to state that if we fix things the way I describe it will work seamlessly with OSO or DWO files. The current state of things is the DWO stuff only uses the fancy DIERef constructor and fills in the dwo number correctly only to have it overwritten in SymbolFile::GetUID(...). The SymbolFile::GetUID(...) is needed for OSOs currently because the DIERef that SymbolFileDWARF (which is used for OSO) doesn't correctly create DIERef objects since they always return llvm::None for SymbolFileDWARF::GetDwoNum(). But the new API will have SymbolFileDWARF::GetFileIndex() to be used instead of SymbolFileDWARF::GetDwoNum(), and the file index will be set correctly for both DWO and OSO files. We can then change DIERef away from DWO specific naming, and have DIERef have a "m_file_index" and "m_file_index_valid" instead of the dwo specific members. As long as both OSO and DWO files can be found from the user_id_t API calls we are all good. Not sure if this addresses all of your issues or not.

If all of your concerns are not clarified above, can you clarify what is still being overlooked? Both Alexander and I are usually thinking we are addressing everything you want, but we obviously still aren't, so restating your remaining concerns would help us get this patch moving.

Well.. I don't believe we have any ASTImporter calls in any of the other pretty printers, so I think this change is good. And if it causes some other issues, then maybe we need to look at solving them in some other way...

In D138618#4092040, @clayborg wrote:

How about we make DIERef constructor always take all the info that is needed to construct the objects correctly:

DIERef(DWARFDie die);
DIERef(SymbolFileDWARF *dwarf, dw_offset_t die_offset); // might not need this one?
DIERef(user_id_t uid);

We might not need all of these. But in this case, we can't incorrectly use the APIs since all of the objects that are needed to fill it in are in the constructor args. We take away the ability to manually fill in the DWO num and other fields. Would that fix the issues you have with this patch Pavel?

I'm sorry Omair, my comment was meant for Jorge, not you. @skipIfXml is not the right decorator here, although it will probably kinda fix the failure (because the bot does not have xml support either). I *think* the compressed section support is controlled by presence of zlib.

I'm pretty sure that's because that bot builds without zlib support. You'll need to add something like @skipIfXmlSupportMissing to this test (I think we don't have a decorator for zlib now, so you'll probably need to add one).

I'm sorry, but that patch does not fix the problem I am trying to point out. In fact, I think it makes things a lot worse.

oops

In D138618#4083481, @clayborg wrote:

We just need to create all DIERef objects using the GetID() from the symbol file as the file index, and we should be able to remove the SymbolFile::GetUID() function now. As long as file index zero is reserved for "vanilla DWARF that doesn't use DWO or OSO we will know the difference. We might want to not have SymbolFileDWARF inherit from UserID at all, and switch over to have SymbolFileDWARF add a virtual function:

uint32_t m_file_index = 0; // Zero means main DWARF file, 1...N identifies the Nth DWO file or OSO file
virtual uint32_t GetFileIndex() { return m_file_index; }

Then anyone can set the file index correctly for DWO or OSO files. And we avoid using user_id_t values for the symbol files since they aren't needed.

I wouldn't exactly use the word happy, but I also don't have the time to come up with an alternative proposal.

In D138618#4077851, @ayermolo wrote:

In D138618#4073329, @labath wrote:

I think that the 1-based index thingy helps a lot here, but I haven't seen anything (in your reponse, or in the new patch) that would address my concernt DIERef<->user_id conversion ambiguity. I.e. how is one supposed to know what is the "right" way to convert a DIERef to a user_id:

• die_ref.get_id()
• or symbol_file.GetUID(die_ref) (which, funnily enough, will construct another DIERef, and *then* call get_id? (return DIERef(GetID(), ref.section(), ref.die_offset()).get_id();)

What's your position on that? That we should live with the ambiguity?

Searching for GetUID doesn't look like it's used all that often, maybe follow up patch is just to get rid of it, and replace with DIERef?

In D138618#4060565, @clayborg wrote:

...
Since the user IDs of SymbolFileDWARF plug-ins mean nothing to anyone else, we can make them what we need them to be so they work for us. I would suggest to remove the use of DIERef from calculating the IDs of symbol files and have .o files for mac and .dwo files for fission use a 1 based index as their ID to make it easy to encode into a DIERef when needed for lldb::user_id_t values that _are_ included in objects that we hand out. Is there anything else that would need to be done to keep everyone happy here?

Thanks for your response, Jim.

I'm sorry, but I still don't feel I can properly review. Can you answer the questions from my previous comment? I need to understand what are the locations of the various lldb components and the values of relevant variables (LLDB_PYTHON_RELATIVE_LIBDIR for one), before and after this patch.

In addition to that, it breaks tests with ubsan. :)

In D68655#4048009, @jasonmolenda wrote:

In D68655#4047126, @labath wrote:

In D68655#4045895, @jasonmolenda wrote:

Both have to be written by the dwarf linker to be correct, but only the former is written ONLY by the dwarf linker.

I don't think that's right:

$ clang -c -x c - -o - -gdwarf-aranges -g <<<"void f(){}" | llvm-readelf - --sections | grep aranges
  [ 6] .debug_aranges    PROGBITS        0000000000000000 0000a0 000030 00      0   0  1
  [ 7] .rela.debug_aranges RELA          0000000000000000 000380 000030 18   I 20   6  8

Ah thanks Pavel, clang on Darwin doesn't emit this in .o files.

It does. It just doesn't do it by default -- same as on linux (but not on PS4 I believe). You have to use the -gdwarf-aranges flag explicitly (which, quite possibly, noone does),

(The fix seems to make LLDB do the right thing, although I can't claim to fully understand what is going on. From what I can tell, we're currently detaching from the fork child when the fork event gets pulled off the public event queue, and this code is preventing that from happening. Intuitively it seems like the public event queue pull is slightly too late to be doing this kind of thing.)

Just to clarify: I was responding to the question about the ELF file usage, not questioning the overall approach. Given that MachO has a special code for dsym files, I think it makes sense to use it.

I don't dispute the value of determinism, but it seems like there ought to be a way to achieve this with corrupting the build tree (which in itself is not very 'deterministic').
What if we had two copies of the framework in the build-tree? One pristine copy, which would only contain liblldb (and any stuff that cmake puts there by default), and then another one which would be used for running, and which would contain all of the manually added stuff (with the right rpaths and all). When installing, one would use the pristine copy as the source instead of the adulterated one.

In D68655#4045895, @jasonmolenda wrote:

Both have to be written by the dwarf linker to be correct, but only the former is written ONLY by the dwarf linker.

Does this mean that the in-tree lldb will cease to be functional once someone "installs" it? (at least until the next rebuild)

@dblaikie, who was looking for aranges vs. DW_AT_ranges comparisons

In D141330#4037932, @JDevlieghere wrote:

In D141330#4037925, @aprantl wrote:

Should this be true for a fully linked ELF executable, too?

Sounds reasonable, but adding @labath and @DavidSpickett to confirm. This is trivial to extend later.

I'm sorry for the delay, I was out for the holidays. Looks now, thanks for your patience.

LGTM

This is looking much better -- and focused. I still have comments though. :)

In D139945#4009427, @JDevlieghere wrote:

In D139945#3999351, @labath wrote:

For a "plugin", the scripted process is sure getting a lot of special handling in generic code. (I know this isn't being introduced here, but I wasn't involved in the previous review -- and I'm not actually sure I want to be involved here). I don't think that's necessarily a bad thing in this case, but maybe we should not be calling it a plugin in that case? We already have a couple of precedents for putting implementations of "pluggable" classes into generic code -- ProcessTrace for instance. And just like in the case of ProcessTrace (where the real plugin is the thing which handles the trace file format), here the real plugin would the the scripting language backing the scripted process?

(Apart from that, this patch seems fine.)

Maybe one way around this is to have some generic metadata that gets passed to the plugin, that can be different per plugin?

I think that the main reason we've arrived at such different conclusions is that I treat the "user IDs of DIEs" and and "user IDs of symbol files" as essentially two different things (namespaces if you will). Obviously, one needs the symbol file ID in order to compute the DIERef ID, but theoretically those two can use completely different encodings. With this take on things, I stand by my assertion that DIERef->user_id conversions are tightly controlled. The symbol file ID computations are a mess.

That looks great. I have just one small nit. I think the test would read better if you made the names of temporary TemplateArgument objects more descriptive -- i.e. encode the kind and value information in the name. For example. int47Arg for an integer template argument whose value is 47, and intTypeArg for a type template argument (where int is the "value").

In D139955#3999381, @ayermolo wrote:

I tried to modify all the places wehre getOffset() is used. Which will later return 64bit instead of 32 bit.

Sorry I guess there was miss communication.
I am not quite clear in what you are suggesting.
The way I did it we now have two distinct interfaces. One for old way that takes in char* and args and applies formatting internally, and a new one which takes in std::string. So all formatting is done on caller side with std::string(llvm::formatv(..)).
Are you suggesting to change implementation of

void ReportError(const char *format, ...)
      __attribute__((format(printf, 2, 3)));

and others to use llvm::formatv under the hood instead?

Yes, that's pretty much it.

For a "plugin", the scripted process is sure getting a lot of special handling in generic code. (I know this isn't being introduced here, but I wasn't involved in the previous review -- and I'm not actually sure I want to be involved here). I don't think that's necessarily a bad thing in this case, but maybe we should not be calling it a plugin in that case? We already have a couple of precedents for putting implementations of "pluggable" classes into generic code -- ProcessTrace for instance. And just like in the case of ProcessTrace (where the real plugin is the thing which handles the trace file format), here the real plugin would the the scripting language backing the scripted process?

I think that test should just be rewritten to not modify the object in such a way. Instead of cumulatively modifying a single object, it could just create a fresh one each time. That will also make it clearer what is being tested. And I think it's fair game to add some constructors to the production object to make that easy.

In D140092#3998835, @Emmmer wrote:

@labath Fixed in rGd850b340a9dd91fdb3c4dddc8328524c721576be

Thanks for splitting this up. We still need to figure out what to do with the first patch, but these two are looking very good now.

Yeah, I'm afraid this is all my fault. I suggested going in this direction, but this isn't exactly how I thought it would turn out. I was expecting that the inferface would be something along the lines of what Jonas posted in his comment. And I was not expecting that you will try to convert every single instance of PRIx32 dwarf offset printing. The majority of the modified lines are the ReportError and ReportWarning statements, so I though we would just be converting those. And the majority of the callers of these functions are coming from DWARF code, so I agree with Jonas that it would be nice to convert all callers of these functions -- but I won't insist on it.

That definitely looks much better. As I understand it, the scripted thread interface is more high-level that our internal thread interface, and that means it is computing its queue by itself, instead of delegating the work to a separate plugin. That makes sense to me.

I just found out that this test (the non-shared-library version) fails on linux if the executable is built with -no-pie (which is the default if the CLANG_DEFAULT_PIE_ON_LINUX option is not set, which happened to be the case for my build). I think the important fact here is that a PIE ELF executable gets its e_type field set to ET_DYN, which causes lldb to identify it as a shared library. However, it is not clear to me why would that matter in this case...

You can just delete the XFAIL line. I probably should have done that when I added UNSUPPORTED: system-linux in 1004d6e7e2eb24edc01d7e330c538ce5cb590001.

I don't think that this indicates a problem with this patch, but you might be interested to know that this broke a test in lldb which was trying to change the PC value to a given line (and it failed because that line was now associated with multiple line entries). I have worked around the problem with a06342d250ec7bee37dc93477f233e43e597aca5 and filed PR59458 to track possible improvements in the lldb implementation.