If you set a breakpoint on source lines with no line table entries, lldb slides the actual match forward to the nearest line table entry to the given line number. That's necessary for instance because if you lay out your function definitions over multiple lines they won't all get line table entries, but we don't want people to have to guess which of them was... Same is true of more complicated compound statements.

How were you setting the breakpoint in the case where it was failing from the command line? The breakpoint you are examining in the test in your patches is a "source regular expression" breakpoint. That would be "break set -p". If you were also doing that in the command line and got a different result that would be very odd, since neither interface (CLI or SB) does much work before handing the request off to the common routine. If (as I suspect) you were doing "b main.c:40" in the command line, then you should make a breakpoint with SBTarget.BreakpointCreateByLocation and see if that shows the same problem. That should do the same job as "break set --file --line".

Thanks for looking into this further! The thing to figure out is who still has a reference to either the Thread * or to the ThreadPlanStack over the destruction of the thread. That shouldn't be allowed to happen.

In D93895#2488372, @clayborg wrote:

In D93895#2488249, @jingham wrote:

looks good to me too. When you get around to the wait times & intervals I'd argue for not doing that as a GDBRemote specific addition, as Greg was suggesting above. There's nothing gdb-remote specific about how long you want the debug agent to wait around for some process to show up.

As long as we are find passing these arguments to _any_ process subclass I am find doing these as options.

looks good to me too. When you get around to the wait times & intervals I'd argue for not doing that as a GDBRemote specific addition, as Greg was suggesting above. There's nothing gdb-remote specific about how long you want the debug agent to wait around for some process to show up.

In D94077#2486225, @werat wrote:

In D94077#2479984, @shafik wrote:

We can have unscoped enums in namespace and class scope and the enumerators won't leak out from those scopes. Thus we can have shadowing going on e.g.:

...

How does this change deal with those cases?

Thanks for pointing this out! My first patch didn't really take this into account, so it didn't work properly. I've made some changes to make it work, although I'm not very familiar with these parts of LLDB yet, so I'm not sure whether this approach is how it should be implemented.

Also I've noticed that LLDB's expression evaluator is not perfect with these lookups, e.g. it can find global variables from other scopes if there's no better candidate (not sure if this is a bug of a feature):

Process 3768045 stopped
* thread #1, name = 'a.out', stop reason = breakpoint 1.1
    frame #0: 0x0000000000401116 a.out`main at global.cpp:2:13
   1    namespace Foo { int x = 1; }
-> 2    int main() {};
(lldb) p x
(int) $0 = 1

LGTM. It looks like we never made use of the distinction between "started to finalize" and "done finalizing", so just marking it at the start of finalization seems fine.

LGTM, thanks for the cleanup.

LGTM - thanks for doing this!

This all looks fine except I'm not sure you can have a single override context. The lldb command line is only a bit recursive, but you can have sequences like:

It's a little awkward that you have the "do_select" parameter with a default argument of "true" but then you explicitly pass "true" every time you use it...

In D92643#2433441, @werat wrote:

In D92643#2433428, @labath wrote:

Are the static members included in the (SB)Type object that gets created when parsing the enclosing type? If yes, we might be able to retrieve them that way. Whether that would be cleaner -- I'm not sure...

(I would expect they are included, as otherwise they would be unavailable to the expression evaluator.)

I think they're not as of now, but there's a patch from @teemperor to add them there -- D81471

Also, I was looking at Type/TypeSystem and it seems there's no API right now to get static members from a Type.

Note, lldb has a bunch of special handling for line 0 code. For instance, when we are pushing a breakpoint past the prologue we will keep pushing it forward over line number 0 lines. Those are compiler generated and in general people don't want to stop there. Similarly, if you are stepping through line 3 and the next line entry after 3 is line 0 we keep stepping till we get to a non-zero line.

LGTM

I don't see any reason for, and lots of reasons against having more than one source of truth for a Debugger's "Currently Selected ExecutionContext". In particular, I can't see any good uses of the Debugger and the CommandInterpreter being able to have different "currently selected targets/threads/frames". For instance, I think people would generally be surprised if calling SBDebugger.SetSelectedTarget didn't also change the default target that subsequent command interpreter commands use.

At present, step-out stops right on the return to the parent frame. That's to support stepping into and out of nested function calls on a single source line. So step-out is often going to end up stopping somewhere a little awkward w.r.t. the line tables, and since there is no standard for where compilers emit line table markers, lldb has to be flexible and handle whatever seems roughly reasonable. It seems strange that should be any more work to do after calling step_out_of_here on that line, so having clang assign the return to the function call line seemed a bit odd. The current clang/icc/gcc behavior certainly makes more sense.

It seems weird that even if you had a summary formatter for some pointer type that was trying to print "DANGER WILL ROBINSON" when the pointer value was 0x0, we will override that and print "nullptr" in a C++ context or "nil" in an ObjC context. Seems like even if the value is Nil we should first consult the ValueObject's summary value and only do the nullptr/nil computation if it was empty.

The attraction of having stub fix up the PC in the stop after hitting the trap for breakpoints (in this case by moving the PC before the trap on architectures that execute the trap) was that this decision could be made simply in the stub, but if lldb had to check DECR_PC_AFTER_BREAK to understand a breakpoint stop, that would need to happen in a bunch of different places (as it did in gdb) and would be easier to get wrong.

It's easier to read these tests if you use "lldbutil.run_to_source_breakpoint" to remove all that boilerplate.

IIUC, the expression parser part of this change suppresses any Fixits that are clang-tidy type rewrites, is that right? If so that is indeed the correct behavior. But the fact that this change implements that behavior is entirely non-obvious at the call site. Could you either add a comment here explaining to point of the test or maybe wrap the test in a method on the FixItHint class so that it's self documenting?

Oh, my bad, apparently we were leaving the dummy target out of the TargetList. That's a little odd, but I probably did it so that's not so unusual... Ignore my comments...

LGTM

The test failed in the new form at line 77. So we got the CU for compile_unit1.c and it was valid. But it didn't contain an SBType for compile_unit1_type. Probably the same thing is true in compile_unit2.c you just didn't get there.

Sorry to be picky, but you can compress this by putting the IsValid checks into your find_* routines. And the:

This test is still assuming (1) that main.c is CompUnit(0), etc. You don't know that's true. And that there is only one type in each CU (and type 0 is the one you want). Neither of these is guaranteed. Can you check all these things, I'd rather not have to go a bunch of rounds on this.

In D88483#2363075, @fallkrum wrote:

@JDevlieghere @jingham api test failed after my commit:
http://lab.llvm.org:8011/#/builders/68/builds/1040
Had no such an error on my computer, please help to figure out what went wrong.

In D78972#2353643, @labath wrote:

Right -- I can understand that. I'm not sure whether this is a real issue -- I'd expect that these compiler-internal symbols would normally have private (not "internal") linkage (and hence be caught by line 330) -- but I'm not insisting on exposing all of the internal linkage symbols either.

However, I think the phrase "meant to be exported" brings us back to the beginning, full circle. Objects with "weak" linkage are also "meant" to be exported (== externally visible in a normal compilation), just like their weak_odr counterparts. I think that the real bug here is that we've started special-casing individual linkage types, and that it's only a matter of time before we find ourselves needing to add another linkage type to this list. Llvm has a bunch of functions to check for the properties of linkage types, and it seems to me like we should be able to pick one of them. So, if we want to just expose the functions that would be visible during a normal compilation, the function to use is hasLocalLinkage() (negation of it, that is)

Switched to !hasLocalLinkage.

In D89157#2362697, @JosephTremoulet wrote:

But then you get to a point where you shouldn't really have multiple modules replacing a single one so you aren't really sure what to do about it. That part makes me a little uneasy.

Yeah, I wouldn't claim that the handling of the multiple-old-module case there is ideal. My thinking is that it makes an incremental step in the right direction, though -- the same potential for having multiple old modules is there with or without the change; the change makes the issue more apparent to readers of the code, makes note of it in the relevant log if we hit the issue at runtime, and refactors the code so there is one place to handle the issue. If you think it's best to hold off on that until we have a better way to actually handle the case, I can see the logic in that, just let me know and I'll rebase this to fix the bug in the meantime.

Thanks!

All the parts of D89156 that are just being consistent about always passing old_modules as a vector seem okay. But then you get to a point where you shouldn't really have multiple modules replacing a single one so you aren't really sure what to do about it. That part makes me a little uneasy.

I can't see anything wrong with this, it seems to follow the practice of other parts of GetSharedModule. I'm still not quite certain why I've never seen an instance where the absence of your fix has caused problems, and it isn't clear from the patch why that is. If there something special about handling breakpad files that moves you out of the normal code path for replacing overridden modules? So I still have a lurking feeling I'm missing something?

This was done on purpose, in commit 902974277d507a149e33487d32e4ba58c41451b6. The comment there is:

In D78972#2041680, @labath wrote:

I don't think we need to have different behavior for repl and --top-level. I'm mainly just confused about what is the right behavior to aim for.

So, assuming we want these to behave as if everything was in a single TU, my next question is: What is the purpose of the external flag? In this setup, it seems like everything should be "external" (i.e., made available to subsequent expressions)...

LGTM. Thanks for making this easier to use.

In D86435#2342826, @lhames wrote:

@jingham @clayborg Do either of you recognize the error that seems to be triggered? It looks like it's consistent across the tests, or at least the ones that I looked at:

FAIL: test_non_cpp_language_dwarf (TestImportStdModule.ImportStdModule)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/mnt/disks/ssd0/agent/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1825, in test_method
    return attrvalue(self)
  File "/mnt/disks/ssd0/agent/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 134, in wrapper
    return func(*args, **kwargs)
  File "/mnt/disks/ssd0/agent/llvm-project/lldb/test/API/commands/expression/import-std-module/basic/TestImportStdModule.py", line 38, in test_non_cpp_language
    lldbutil.run_to_source_breakpoint(self,
  File "/mnt/disks/ssd0/agent/llvm-project/lldb/packages/Python/lldbsuite/test/lldbutil.py", line 953, in run_to_source_breakpoint
    return run_to_breakpoint_do_run(test, target, breakpoint, launch_info,
  File "/mnt/disks/ssd0/agent/llvm-project/lldb/packages/Python/lldbsuite/test/lldbutil.py", line 875, in run_to_breakpoint_do_run
    test.assertEqual(process.GetState(), lldb.eStateStopped)
AssertionError: 10 != 5

I don't know how this JIT Profiling code is wired into the regular JIT mechanism. I assume it is opt-in, in which case it shouldn't have any effect on expression parsing. If it isn't opt in, we should have a way to opt out on the "don't do work whose outcome you don't care about" principle.

In D88483#2341538, @fallkrum wrote:

Thanks a lot Jim for explanations, now it makes sense to me.
Have one more question on class Type. Writing API test for the patch defined 2 functions like this:

void *func1(int) {
        return NULL;
}

void *func2(int) {
        return NULL;
}

Tried to find function type using target.FindFirstType('func1') and got nothing as a result. Call to module.GetTypes().GetTypeAtIndex(0).GetName() contained:
"void *(int)". Looking through clang::Decls found out that clang::FunctionDecl is child of clang::ValueDecl and it finally became clear to me that "func1", "func2" are not types at all but simply symbols (variables?) of the same type "void *(int)". Therefore module where above 2 functions were declared contained only 1 type "void *(int)". Investigating how DWARFASTParser works noticed that it do generates 2 Type instances for 2 defined functions with m_name variable set to "func1", "func2" respectively and m_compiler_type referencing the same "void *(int)" (but not in it's string representation of course).
So the question is: Does class Type represents not only a type but also a variable (symbol or identifier, don't know how to name it correctly in this situation)?

In D88483#2339845, @fallkrum wrote:

In D88483#2339256, @jingham wrote:

In D88483#2338408, @fallkrum wrote:

Can you please give me a couple of words on what m_static_type and m_dynamic_type is in TypeImpl class, in what cases can it be initialized with both of those types?

ValueObjectDynamic value uses a TypeImpl to store its type, and it stores both the static type (the one returned by the parsed expression or was in the DWARF variable records for some local value) and the dynamic type it figured out by looking at the value in memory in this TypeImpl. See for instance ValueObjectDynamicValue::UpdateValue. ValueObjectDynamicValue could also get its static type from its parent. I'm not sure why it doesn't do that and stores it in its TypeImpl instead; presumably this was more convenient somewhere.

Thanks for explanation.
Could please tell if I understood you correctly: from the start we load into some module’s type system some type for example from DWARF, but during runtime it may be altered and as a consequence ‘original’ static type becomes dynamic in-memory type? It still seems to me I do not completely understand relation between static and it’s dynamic part, if it is correctly to say so. Would like to understand the origin of this.

In D88483#2338408, @fallkrum wrote:

Can you please give me a couple of words on what m_static_type and m_dynamic_type is in TypeImpl class, in what cases can it be initialized with both of those types?

LGTM

That's nicer, thanks!

Use lldb-instr instead of hand-generating the reproducer wrappers.

Thanks, that's a little better.

IIUC, the problem with ReplaceModule will get fixed in a dependent patch, right? So the last bit of this patch will get cleaned up by that change. If that's right, then this LGTM.

The return from a SIGSEGV or SIGILL has less information than the equivalent exception, so reporting the exceptions should be the default, but having a switch for people who need the exception to propagate is okay.

The change makes sense.

Adopt Pavel's suggestion to do "nosuppress/nostop/notify" for SIGCONT instead of "nosuppress/nostop/nonotify".

In D87868#2309260, @clayborg wrote:

In D87868#2308515, @labath wrote:

That sounds like a plan. FWIW, here's the implementation I hacked up today:

Status NativeProcessLinux::AllocateMemory(size_t size, uint32_t permissions,
                                          lldb::addr_t &addr) {
  PopulateMemoryRegionCache();
  auto region_it = llvm::find_if(m_mem_region_cache, [](const auto &pair) {
    return pair.first.GetExecutable() == MemoryRegionInfo::eYes;
  });
  if (region_it == m_mem_region_cache.end())
    return Status("No executable memory region found!");
  addr_t exe_addr = region_it->first.GetRange().GetRangeBase();

  NativeThreadLinux &thread = *GetThreadByID(GetID());
  assert(thread.GetState() == eStateStopped);

  int prot = PROT_NONE;
  if (permissions & ePermissionsReadable)
    prot |= PROT_READ;
  if (permissions & ePermissionsWritable)
    prot |= PROT_WRITE;
  if (permissions & ePermissionsExecutable)
    prot |= PROT_EXEC;

  NativeRegisterContextLinux &reg_ctx = thread.GetRegisterContext();
  DataBufferSP registers_sp;
  reg_ctx.ReadAllRegisterValues(registers_sp);
  uint8_t memory[2];
  size_t bytes_read;
  ReadMemory(exe_addr, memory, 2, bytes_read);

  reg_ctx.SetPC(exe_addr);
  reg_ctx.WriteRegisterFromUnsigned(lldb_rax_x86_64, SYS_mmap);
  reg_ctx.WriteRegisterFromUnsigned(lldb_rdi_x86_64, 0);
  reg_ctx.WriteRegisterFromUnsigned(lldb_rsi_x86_64, size);
  reg_ctx.WriteRegisterFromUnsigned(lldb_rdx_x86_64, prot);
  reg_ctx.WriteRegisterFromUnsigned(lldb_r10_x86_64,
                                    MAP_ANONYMOUS | MAP_PRIVATE);
  reg_ctx.WriteRegisterFromUnsigned(lldb_r8_x86_64, -1);
  reg_ctx.WriteRegisterFromUnsigned(lldb_r9_x86_64, 0);
  WriteMemory(exe_addr, "\x0f\x05", 2, bytes_read);
  PtraceWrapper(PTRACE_SINGLESTEP, thread.GetID(), nullptr, nullptr);
  int status;
  ::pid_t wait_pid = llvm::sys::RetryAfterSignal(-1, ::waitpid, thread.GetID(),
                                                 &status, __WALL);
  assert((unsigned)wait_pid == thread.GetID());
  addr = reg_ctx.ReadRegisterAsUnsigned(lldb_rax_x86_64, -ESRCH);

  WriteMemory(exe_addr, memory, 2, bytes_read);
  reg_ctx.WriteAllRegisterValues(registers_sp);

  if (addr > -4096)
    return Status(-addr, eErrorTypePOSIX);
  return Status();
}

It needs more error handling, and generalization to non-x86 architectures, but it actually works, and is enough to make all but one test pass (TestBitfields.py seems to fail due to some data corruption -- quite puzzling).

Very cool. That seems simple enough to allow us to try this out, at least on unix variants. Does PTRACE_SINGLESTEP cause _only_ the current thread to single step and keep all other threads paused? We can't allow any other threads making any progress when the process is briefly resumed in a multi-threaded process.

This nice thing is lldb-server _is_ compiled natively for each system so we can rely on system headers for the sys call numbers if they are available. So looks like this could be made to work. Did you also do the deallocate memory? I ran a few expressions and I am not sure that we ever use the deallocate memory packets to debugserver on mac, so it might not be needed.

Architecture is more specific to the structural aspect of a thing, whereas Design is more general. For instance, the rules for the SB API don’t really seem like a description of an Architecture, but more some general principles for doing the thing.

I like the name Design better than Architecture for the top level. For instance the structureddataplugins is not really Architecture, it's the design of a fairly small component of lldb

Ah, Adrian is right, you should probably move Architecture to Design.

LGTM

We should probably outlaw the shared pointer from weak pointer constructor in lldb. It looks like the only safe way to use it is in a try/catch block (or by preflighting the weak pointer with lock which renders it pretty much pointless.)

Use std::weak_ptr::lock to make the shared pointer, rather than std::shared_ptr::shared_ptr(std::weak_ptr) (sort of), as the latter throws.

In D88266#2301934, @clayborg wrote:

Jim: were you able to repro this with a simple a.out program? Is there something worse going on here like maybe incompatible libc++/libstdc++ implementations?

Check safely for an unset plugin, then try to get the plugin and if either of those fails, use the underlying ObjectSP's Dump method to dump the data.

Jonas just revived the long Doc that Todd wrote about the Structured Data printing plugins. Seems like that is a useful thing, so I'm not in favor of getting rid of it just yet.

But, sure, if nobody can come up with a use for this plugin dealie, I'll get rid of it. Otherwise I guess we should make a default plugin that calls "Dump"?

I am happier removing things when I know what they were for??? Not really sure why this plugin architecture is there. My guess is it is part of Todd's os_log facility, but I'm not really sure.

Addressed review comments.

Addressed review comments.

I agree that having --disable flip the meaning of the listed breakpoints is a little odd, but I think its the most useful meaning by far.

In the original version of the patch you couldn't supply any breakpoints along with the --disabled flag. But it makes sense to be able to have some breakpoints that don't get deleted by this operation, which you can achieve by passing excluded breakpoints, so:

Added docs to the python-reference.rst