User Details
- User Since
- Jun 4 2013, 6:02 AM (511 w, 5 d)
Tue, Mar 21
Tue, Mar 14
It's not exactly what I had in mind, but I kinda like it :)
yay
Tue, Mar 7
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).
Mon, Mar 6
I kinda like this.
Wed, Mar 1
Feb 23 2023
Feb 21 2023
Feb 20 2023
Nice catch.
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.
Feb 13 2023
Thank you for your patience. I'm really happy with the overall result here.
Feb 8 2023
Feb 3 2023
I like this.
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...
Feb 2 2023
Jan 31 2023
Jan 30 2023
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).
Jan 27 2023
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
I wouldn't exactly use the word happy, but I also don't have the time to come up with an alternative proposal.
Jan 25 2023
Jan 23 2023
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.
Jan 13 2023
In addition to that, it breaks tests with ubsan. :)
Jan 12 2023
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.
Jan 11 2023
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
Jan 9 2023
I'm sorry for the delay, I was out for the holidays. Looks now, thanks for your patience.
Dec 22 2022
LGTM
Dec 21 2022
This is looking much better -- and focused. I still have comments though. :)
Dec 19 2022
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").
Dec 15 2022
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.
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.
Dec 13 2022
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.
Dec 12 2022
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.