- User Since
- Jun 4 2013, 6:02 AM (381 w, 21 h)
Thu, Sep 17
This sounds like a good idea, but I'd like to get another set of eyes on the llvm part.
Wed, Sep 16
Ok, this is something we can talk about. dwarf parsing is a big bottle neck, so spending some time to optimize that (and even sacrifice some readability/mantainability/etc. if needed) is worthwhile.
Tue, Sep 15
This is fixing the same issue that I tried to fix with D84402, but then failed to get back to it when the discussion about the best approach started getting long. While I do have some reservations about this approach, it is definitive improvement than the status quo, so I won't start bikeshedding the design (but do see the inline comments)..
@tstellar, who is (was) the release manager for 10.0, can make a definitive call, but our usual modus operandi is to have one point release for each major version. I don't know if this is a sufficiently major issue to break from that (and it definitely is risky).
Using emplacement instead of construction is a good idea, where it makes sense. However, I am not convinced that adding constructors to otherwise-trivial structures is worth it. These structures are usually trivially copyable and/or movable and so an emplacement does not really save much.
(Sorry about the delay.) Given the current requirements, I think this patch is fine (excellent even).
This looks fine (sorry about the delay, I've been OOO).
Fri, Sep 4
I'm not married to the current way we process commands, but I do value their consistency (both between different commands, and between a command and their documentation). This would make script behave differently than expr even though they have identical modes of operation (raw input, special handling of empty argument, etc.), and differently from its documentation. The help script command contains this:
Invoke the script interpreter with provided code and display any results. Start the interactive interpreter if no code is supplied. Expects 'raw' input (see 'help raw-input'.)
I'm not sure how this is patch coded, but probably after the argument are added, this additional snippet will appear:
Important Note: Because this command takes 'raw' input, if you use any command options you must use ' -- ' between the end of the command options and the beginning of the raw input.
help raw-input says this:
<raw-input> -- Free-form text passed to a command without prior interpretation, allowing spaces without requiring quotes. To pass arguments and free form text put two dashes ' -- ' between the last argument and any raw input.
If we do want to change that, we should at least make sure all of these things reflect that. And yeah, that should be a separate patch (maybe a small RFC even).
Thu, Sep 3
I'm pretty sure the json changes should get a separate review. The json owner (let's call him @sammccall) might have a different idea on how to do this thing...
I'd like to also pull in Jason for this, since this is really the trickiest part of the whole patchset.
There are things I'd want to change here, but I think this is patch is fundamentally ok. Before going into that, I think we should first sort out the dependant patch.
Wed, Sep 2
I don't know anything about Sphinx. It might be useful to mention in the patch description the motivation for this change.
This is great, thanks for doing this. Just a couple of minor comments inline.
Thanks, this looks good to me now.
I like this. And a big thank-you for writing the tests. We don't usually bother to write self-tests for the test infrastructure, but we definitely should be doing that. The place is slightly weird, but I think it will do for now -- it's easy to move this around if we find a better place for it.
Fri, Aug 28
I'm going to respond to the rest of your (very insightful) comment later. So far, I'm just responding to this:
This isn't exactly layout related, but there is the question of covariant methods. If a method is covariant, then its return type must be complete.
We already import all the base classes of a derived class (at least in full import). But, we do not import all the derived classes during the import of the base. Is this what you do in LLDB to support covariant return types?
I think it would be appropriate to discuss the design of this feature on lldb-dev before going over the individual patches. One of the fundamental aspects of this patchset that I think should not be overlooked is that it essentially adds a new level of structure ( a "Target Group") lldb. One of the questions I'd have is whether this concept shouldn't be formalized somewhere instead of it existing just virtually as the group of threads that happen to share the same trace object.
This is going to derail this patch somewhat (sorry), but since this is an important long-standing open issue, I think we should discuss it in more detail.
This looks like it could work, though only barely. @skipIfWindows checks for target system, but we would really need this to check the host OS (for remote testing, although I don't know if anyone does that on windows these days). However, in conjuction with @skipIfRemote it should do the right thing. In any case, this is small enough a change to try it out. Just be sure to check the windows bot afterwards.
Thu, Aug 27
If the format is going to be json, why not use llvm::json from llvm/Support/JSON.h? That's what we been migrating most of the existing stuff to already, so going using that for new code makes perfect sense.
I am wondering what should the platform classes which do not implement this feature do (although most of them could implemented I don't think you need to implement all of them -- PlatformGdbRemote in particular can be a bit tricky). It seems to me like it would be better for them to bail out if a non-standard shell is requested instead of silently executing a command the user did not intend.
Wed, Aug 26
Sorry, I missed this patch. Modulo the inline comments and the missing windows support and tests, I think this patch is ok-ish. However, I am still not convinced of its usefulness. Why would we be doing something (particularly a thing which will be hard to do in a cross-platform manner, and will very likely border on, or downright cross into, undefined behavior territory), if we get that from vscode for free?
Thanks for the clarification. The new version looks great.
Though these extra messages are often superfluous in general, I don't think that's the case for this particular message. This macro is only used in the expect and match checks. These are the most common way of asserting something right now, so I think it's reasonable to spend some effort to make the errors it produces clear and actionable. The current state is pretty far from ideal.
Sounds good. FWIW, I wouldn't be opposed to some CLI command which would dump some low-level details of the register context, similar to how we have the "target modules dump" command tree for Module objects...
This looks good, though it would be nice to add a test for it. It looks like TestGdbRemoteExpeditedRegisters.py has a lot of functionality that could be reused for this purpose.
I like this a lot. LGTM with some small fixes inline.
Mon, Aug 24
Aug 24 2020
Sorry about the delay. This looks good to me now.
If I correctly understand what this is doing, then I don't think it's a good idea. The base of an (elf) shared library does not have to be mapped executable. These are the mappings I get for a trivial hello world program (no mmapping of libraries or anything) on my linux machine:
00400000-00401000 r--p 00000000 fd:01 2838574 /tmp/l/a.out 00401000-00402000 r-xp 00001000 fd:01 2838574 /tmp/l/a.out 00402000-00403000 r--p 00002000 fd:01 2838574 /tmp/l/a.out 00403000-00404000 r--p 00002000 fd:01 2838574 /tmp/l/a.out 00404000-00405000 rw-p 00003000 fd:01 2838574 /tmp/l/a.out 020fb000-0211c000 rw-p 00000000 00:00 0 [heap] 7fe4f5d87000-7fe4f5da9000 r--p 00000000 fd:01 2738932 /lib64/libc-2.31.so 7fe4f5da9000-7fe4f5ef3000 r-xp 00022000 fd:01 2738932 /lib64/libc-2.31.so 7fe4f5ef3000-7fe4f5f3d000 r--p 0016c000 fd:01 2738932 /lib64/libc-2.31.so 7fe4f5f3d000-7fe4f5f41000 r--p 001b5000 fd:01 2738932 /lib64/libc-2.31.so 7fe4f5f41000-7fe4f5f43000 rw-p 001b9000 fd:01 2738932 /lib64/libc-2.31.so ...
Here, the correct base of a.out is 0x00400000 and the libc base is 0x7fe4f5d87000. But this patch would make them be detected as 0x00401000 and 0x7fe4f5da9000, respectively.
I actually think tests relying on this should be converted to c++ unit tests. The logic to compile and link against liblldb takes up a considerable chunk of our test buildsystem. Why recreate that logic, when cmake knows how to do that already? TestPluginCommands might be more involved, but converting api/command-return-object should be trivial using the api unit test framework we already have (unittests/API)...
Aug 21 2020
Actually, keep the old const_value test, but give it a bitfield-specific name. Testing bitfields+const_values is still interesting.
LGTM, modulo the defensive check. Incidentally, I was just looking at a DW_AT_const_value bug, but I think it's a different issue than this one.
Aug 20 2020
If you start to need to make more changes for your project, we'll need to have a bigger discussion, but right now, this seems ok.
Also, +@markmentovai, in case he has any thoughts on this.
This has been a feature we've been missing for a while. Thanks for implementing it.
Aug 19 2020
I looked at the gdb-remote docs, and I could find precedents for both returning error (or OK) for "empty" responses, and prefixing the responses with a random character. So I think adding the M character is fine too.