User Details
- User Since
- Jun 4 2013, 6:02 AM (398 w, 3 d)
Today
I should add, this also ensures one does not need to introduce a similar amount of cruft for each new llvm tool that he wants to use in the tests.
There's still one "opportunistic" snippet in GDBRemoteRegisterContext.cpp, around line 404. Did you forget about that, or is there something special about that case? (if so, then what?)
The fact that the test suite passes is not too surprising since we have very few core file tests. However, I agree that we should just remove this zero filling completely.
Can you be more specific about what is the nature and cause of this breakage?
Wed, Jan 20
Tue, Jan 19
(sorry for inactivity -- I mostly tuned out of this discussion as I
couldn't keep up with its pace)
It looks like this is removing the ability to build libc++ tests with gcc (as it does not have the -stdlib option). While having that ability would be nice, I don't believe there's anyone currently using that configuration, so it shouldn't stand in the way of other things. But we should also update the python detection code then (in canRunLibcxxTests in packages/Python/lldbsuite/test/dotest.py -- I guess you just need to remove the if os.path.isdir("/usr/include/c++/v1"): blurb)
Mon, Jan 18
Looks like a nice cleanup. The only part I am not sure of is the part about removing $(RM) $(ARCHIVE_OBJECTS). Is that necessary?
I'm not sure why is that line there, but if I had to guess, I would say it's to ensure that lldb (on macos) reads debug info from the archive file instead of the original .o files. If it's not required, it may be better to leave it in. Otherwise, someone from Apple should say whether that is ok (testing archives is only really interesting on fruity platforms).
Looks good, thanks.
Sun, Jan 17
Fri, Jan 15
Yeah, this is fine. I remember running this on darwin and noticing that debugserver only supports G (and lldb-server, amusingly, only supports g). I must have forgotten to add the decorators when copying patches around. Sorry.
Thu, Jan 14
Thanks for your patience. I think this is in a pretty good shape now. Just a couple of quick questions inline...
Using remote numbers in the lists seems reasonable as well, but I am troubled by the "opportunistic" aspect of the implementation. This pattern
uint32_t lldb_regnum = ConvertRegisterKindToRegisterNumber( eRegisterKindProcessPlugin, reg); if (lldb_regnum != LLDB_INVALID_REGNUM) reg = lldb_regnum; const RegisterInfo *value_reg_info = GetRegisterInfoAtIndex(reg);
is cumbersome to write and easy to get wrong.
Wed, Jan 13
Mon, Jan 11
Sun, Jan 10
Looks good, modulo the inline comment.
Fri, Jan 8
Thu, Jan 7
Yes, it seems it is, though just by a bit. :)
Wed, Jan 6
Thanks. This talk of all these packets has made me realize I did not completely understand the purpose of this packet. To really test the exclusion functionality of this packet, I guess we should be launching two instances of the inferior (one before sending the packet, one after), and then checking we attach to the right one. It looks like it should be easy to add to the existing test. Other than that (and some inline simplifications) this looks fine to me.
I think this is fine -- sorry this dropped off my radar.
LG, modulo inline comment.
I don't think it's a good idea to stuff all of this into a single patch. Can we go back to the version which just implements the basic vAttachWait packet (we can bikeshed on what the default interval should be)? I believe new commands/options/packets should be done in separate patches...
This suffers from the same problem as the other patch, where the other index classes (apple_names and debug_names) will essentially never be able to implement this feature. (Unless they re-index the dwarf themselves, that is, but this would defeat the purpose of having the index in the first place.)
I don't think that simply setting func_lo_pc to zero will be sufficient to make this work. I would expect this to break in more complicated scenarios (like, even if we just have two of these functions). I think the only reason it works in this case is because for this particular function, it's base address (relative to the CU base) *is* zero.
The fix is good, but the test could be improved. Combining assembly input with running the inferior effectively limits the test to a single platform (assembly is not portable, and running requires asm to match the host). AFAICT, we don't actually need to run the binary to test this fix -- checking just the "breakpoint set" command should suffice (if you want to be more explicit in what is being checked, you can also run "breakpoint list -v" and test that). Then you'd just need to replace %clang_host with %clang -target x86_64-pc-linux (or something) and UNSUPPORTED: system-windows with REQUIRES: x86.
This looks better. I haven't checked the rest of the patch in detail, but it seems ok at a quick glance and Jonas appeared to be happy with it.
Sat, Jan 2
Fri, Jan 1
Have you by any chance learned why are we zero-filling this buffer in the first place? Seems like an odd thing to do... Maybe we should just stop zero-filling completely?
The new version seems ok-ish to me, but I'm leaving it open as I'd like Jim to weigh in here. The immediate cause of the crash is related to the (dangling?) m_thread pointer inside the completed "step instruction" plan. Normally, this pointer clears itself when the thread disappears but I guess this does not happen in the exec case. Jim should know what's the expected behavior here.
Thanks for picking this up.
Tue, Dec 29
It seems Thread plans already have logic to handle "exec" events. It would be better to figure out why that logic is not sufficient instead of adding another exec handling hook.
Mon, Dec 28
Sun, Dec 27
Dec 22 2020
Thinking about this further, I am not sure if the approach with multiple alternatives is a good solution (for this particular problem at least). If we take x87 long double constants (currently not supported here, but one of the things I wanted to look at) for example, they are 10 bytes long. Representing them with DW_OP_implicit_value is trivial. However, if we wanted to represent them with DW_OP_const, we would need to break it down into smaller chunks, something like DW_OP_const low64, DW_OP_stack_value, DW_OP_piece 8, DW_OP_const high16, DW_OP_stack_value, DW_OP_piece 2. If the DIExpression is empty, then that's fairly easy. However, if the expression already contains some operators (DW_OP_LLVM_fragment in particular), then I fear things would get messy. How messy depends on what is actually allowed to be contained in these expressions, which brings me to my next question:
Thanks for the review, and for the explanation Adrian. I want to fix a couple more issues in the general vicinity of this code, so I'll keep this design goal in mind.
You can't put these declarations inside do {} while(0) -- the additional scope works against us in this case, and forces a premature timer termination. Other than that, this seems fine...
Dec 21 2020
It would be nice to "mirror" the test case (test/Shell/ObjectFile/ELF/section-types.yaml) too. Other than that, this is fine.
Dec 20 2020
I've included two logs, showing failing and successful runs (the successful run was captured without this patch). Interesting things happen around line 1057, where the "step out" plan decides we should not stop after stepping out. I'm not sure what's the relation to this patch, but I suppose it could have something to do with it not (always) fetching the correct frame (from which it should step out) from the execution context.
Dec 18 2020
With this version of the patch, I am unable to reproduce the issue using the approach I described in the previous comment. However, it still reproduces when issuing the equivalent commands via the "gui" (either "by hand" or by running TestGuiBasicDebug.py).
Dec 17 2020
I think that streamlining the packet (de)serialization process is badly needing. I'm not so sure about the approach though. On one hand, it's definitely an improvement. On the other, it does not handle the deserialization process, and I was hoping we could have a solution which handles both, for two reasons:
- it makes things simpler
- it allows us to avoid the need for tests which explicitly test serialization and deserialization of each packet, as the things is implemented (and tested) once, centrally.
Sorry for not keeping track of this patch -- I'm not very active these days.
Looks good.
The fix seems reasonable but it would use a test case. Is it possible to generate appropriate input with yaml2obj? If so you could run lldb-test symbols on the resulting object file, and verify that the Symbol table is printed out correctly (whatever "correct" means in this case).
This seems suspicious to me, on a couple of levels. You claim that BOOL is an unsigned type, but the apple documentation says "BOOL is explicitly signed so @encode(BOOL) is c rather than C even if -funsigned-char is used." Which one is true? Could this have something to do with the fact that the documentation assumes obj-c (which does not have a native bool type) but in lldb, we use obj-c++ (which, I guess, inherits bool from c++) everywhere?
I wouldn't dismiss the jetliner idea completely. With all the recent upsets in the aviation industry, there may be an opening for us yet. :P
Let's give this another shot.
Dec 14 2020
Dec 13 2020
Dec 10 2020
Thanks for writing this.