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.

In D95165#2515232, @augusto2112 wrote:

@labath, you were absolutely correct! It was simply a matter of saving and restoring the terminal struct on the terminalHasColors function in the Process.inc file (I really should've tried that before). I'm currently recompiling and will re-run the tests locally, and will push the changes after that. I do worry this could potentially impact macOS though (I don't know if these low-level terminal functions work differently between differently OSes), so how do we ensure this doesn't break anything there?

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?)

In D94888#2511675, @rupprecht wrote:

In D94888#2506140, @labath wrote:

I am pretty sure they were. This flag is needed to use (system) libc++ with gcc, and this was happening at a time when we were running the test suite with gcc, as it was still the default compiler for android.

Is anyone running tests in this build mode (gcc + libc++) still? Should we keep this support around?

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?

In D94888#2506000, @MaskRay wrote:

In D94888#2505992, @labath wrote:

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)

As for testing against the system libc++ with clang, I guess that should still work, as the extra rpath will be just ignored in that case...

I do not know whether the following few lines D9426 were intentional.

(sorry for inactivity -- I mostly tuned out of this discussion as I
couldn't keep up with its pace)

In D94888#2505698, @dblaikie wrote:

Out of curiousity did you find a way to get lldb-test to print out the stdout/stderr of the underlying lldb process to see what error messages it was producing that demonstrated this failure? I was trying to figure out how to do that, and it seems like a good thing to be able to do for this issue or others down the way.

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)

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.

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.

In D94077#2489064, @werat wrote:

Thanks for the explanation, this makes sense. I've checked the mailing list archives and it seems there was already a discussion about the enumerators in the .debug_names index back in 2018 -- http://lists.dwarfstd.org/pipermail/dwarf-discuss-dwarfstd.org/2018-April/004443.html. You were the one to bring it up and the consensus was that the enumerators should go into the index too.

But it seems this was never actually implemented, the latest version of the toolchain doesn't add DW_TAG_enumerator entries to the .debug_names index. Is there a reason for that or it just slipped through the cracks? Should I bring it up again on the mailing list or we can assume the consensus is still the same and this should be just implemented?

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.

Looks good, modulo the inline comment.

In D94077#2486232, @werat wrote:

In D94077#2481942, @labath wrote:

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'm not sure I completely understand. Why does changing the manual_dwarf_index mean that the other index classes can't ever implement this feature? If, let's say, debug_names decides to support enum constants, then its data layout should be changed to either include enumerators as globals, or add a new section, or something else. Then it can be properly handled in LLDB too (maybe changing the way manual_dwarf_index works too to keep things consistent). As I understand, manual_dwarf_index can be changed anytime, since we're building it ourselves.

Do I misunderstand how these indexes work and interact with each other?

In D94063#2483546, @dblaikie wrote:

If it's better to write it using C++ source and custom clang flags I can do that instead (it'll be an -mllvm flag - looks like there's one other test that does that: lldb/test/API/lang/objc/forward-decl/TestForwardDecl.py: dict(CFLAGS_EXTRAS="-dwarf-version=5 -mllvm -accel-tables=Dwarf"))) - means the test will be a bit more convoluted to tickle the subprogram ranges, but not much - just takes two functions+function-sections.

In D94064#2483578, @dblaikie wrote:

In D94064#2481925, @labath wrote:

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.

I certainly don't have high confidence in the change, to be sure - but I think it's a bit more robust than that.

Yes, it seems it is, though just by a bit. :)

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.

In D93895#2481990, @augusto2112 wrote:

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

Ok. I can open other patches after this one is merged.

In D91847#2410723, @krytarowski wrote:

I would mark stmmX as an alias to stX and keep stX as the default for all platforms. stmmX could be an alias for everybody for legacy reasons.

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

In D93926#2479749, @JDevlieghere wrote:

Your argument is correct because the interactive script interpreter always belongs to a single debugger.

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.

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.

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.

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

It would be nice to "mirror" the test case (test/Shell/ObjectFile/ELF/section-types.yaml) too. Other than that, this is fine.

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.

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

In D92013#2453364, @aprantl wrote:

In D92013#2413734, @labath wrote:

I'd like to also fix this in a more fundamental way, but I think I'd need some guidance from someone more familiar with this code (who would that be?).

One idea I had is to add something like isPureFragment() to the DIExpression class, which would return true if the expression consistent solely of a single DW_OP_LLVM_fragment operand.

You mean DIExpression(DW_OP_LLVM_fragment 0, 1) is "pure" s opposed to DIExpression(DW_OP_deref, DW_OP_LLVM_fragment 0, 1), which isn't? Or something else?

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.

In D92187#2451013, @mgorny wrote:

In D92187#2450976, @labath wrote:

In D92187#2450526, @mgorny wrote:

Another fix: we should only unload duplicate ld.so if it's actually a duplicate, i.e. the path differs. Otherwise, we've ended up unloading the only copy.

Would comparing the module shared_pointers work? I think that's exactly what we want here. I think it's possible for LoadModuleAtAddress to return the same module, even with paths which don't match exactly. But if it does that, then we will still end up unloading the original interpreter module...

How do you suggest we get the shared pointer for comparison? Store in class instance, or search for it somehow?

In D92187#2450526, @mgorny wrote:

Another fix: we should only unload duplicate ld.so if it's actually a duplicate, i.e. the path differs. Otherwise, we've ended up unloading the only copy.

Thanks for writing this.