lgtm

In D35223#834050, @emaste wrote:

With this patch I observed three new failures on FreeBSD and three new unexpected passes on FreeBSD. An example of a new failure:

======================================================================
FAIL: test_inferior_crashing_expr_step_and_expr_dwarf (TestInferiorCrashing.CrashingInferiorTestCase)
   Test that lldb expressions work before and after stepping after a crash.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tank/emaste/src/llvm/tools/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1732, in dwarf_test_method
    return attrvalue(self)
  File "/tank/emaste/src/llvm/tools/lldb/packages/Python/lldbsuite/test/decorators.py", line 110, in wrapper
    func(*args, **kwargs)
  File "/tank/emaste/src/llvm/tools/lldb/packages/Python/lldbsuite/test/functionalities/inferior-crashing/TestInferiorCrashing.py", line 82, in test_inferior_crashing_expr_step_and_expr
    self.inferior_crashing_expr_step_expr()
  File "/tank/emaste/src/llvm/tools/lldb/packages/Python/lldbsuite/test/functionalities/inferior-crashing/TestInferiorCrashing.py", line 249, in inferior_crashing_expr_step_expr
    self.check_stop_reason()
  File "/tank/emaste/src/llvm/tools/lldb/packages/Python/lldbsuite/test/functionalities/inferior-crashing/TestInferiorCrashing.py", line 93, in check_stop_reason
    STOPPED_DUE_TO_EXC_BAD_ACCESS)
AssertionError: 0 != 1 : Process should be stopped due to bad access exception
Config=x86_64-/usr/bin/cc
----------------------------------------------------------------------
Ran 7 tests in 4.320s

From lldbsuite/test/lldbutil.py:

def is_thread_crashed(test, thread):
    """In the test suite we dereference a null pointer to simulate a crash. The way this is
    reported depends on the platform."""
    if test.platformIsDarwin():
        return thread.GetStopReason(
        ) == lldb.eStopReasonException and "EXC_BAD_ACCESS" in thread.GetStopDescription(100)
    elif test.getPlatform() == "linux":
        return thread.GetStopReason() == lldb.eStopReasonSignal and thread.GetStopReasonDataAtIndex(
            0) == thread.GetProcess().GetUnixSignals().GetSignalNumberFromName("SIGSEGV")
    else:
        return "invalid address" in thread.GetStopDescription(100)

Presumably we want the second case to apply on FreeBSD as well when this patch is in, although I don't see why the existing else case shouldn't continue to work correctly.

I looked at this briefly last week but I could not find a good fix in the amount of time I had left. This fixes the current test failure, but it does not fix the underlying bug, which is that we (sometimes?) set the compile unit offset for non-dwo, non-dsym DIEs as 0. This works fine if all compile units are regular dwarf as we have logic to locate the containing CU if the offset is zero. However, it can fail in case of mixed dwarf and dwo compile units. My guess is you could still reproduce this bug with a so file that has a dwo compile unit at offset zero.

I am not able to test this right now. Eugene, can you take a look?

In D35607#814982, @clayborg wrote:

So we should just make sure this works:

(lldb) target symbols add --shlib a.out debug_binaries/a.out

The --uuid option is there to help you match up without having to specify the file path. If I am understanding correctly, "a.out" has no UUID or build ID, but "debug_binaries/a.out" does?

In this case, neither of the files has a build id/UUID. However, the tricky part here is that in this case we "invent" a UUID by doing a checksum of the whole file. This is done to support the .gnu_debuglink https://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html method of finding symbols. It works like this:

• if a file has a gnu_debuglink section, we set the UUID to the crc inside that section
• if a file does not have a gnu_debuglink section, we *assume* it will be used as a gnu_debuglink target, and also set the UUID to the crc we compute from the whole file contents.

This works fine for the gnu_debuglink case, as then the rest of the machinery will match the files based on their UUIDs. However, it does not work for the general case without the debug link, as then the files will end up with two different "UUID"s (because they are based on the file content), and everything will consider them as different.

In D35311#814941, @jingham wrote:

This isn't about this patch, but replying to:

debugserver replies to the k packet instead of just dropping the connection - stopping code adjusted, although we should probably consider aligning the behavior of the two stubs in this case

Especially when dealing with flakey connections, knowing that you are done with a target was really handy. People what to hit "rerun" in some UI and have the rerun happen instantly, but if you have no way of knowing when the former process actually died, you have to introduce delays, which slows this down. This is particularly true when talking to an OS that really wants only one copy of an "app" running at a time (e.g. iOS) but is more generally useful for responsiveness. And if something goes wrong with killing the target, we could even use the new error reply strings to give more info about this.

In D35622#814717, @clayborg wrote:

If you can forward a DWARF file that exhibits this behavior I can take a look.

@beanz: It looks like you're doing some debugserver work. :) Any thoughts on this?

Btw, will this work correctly if we are cross-debugging from a mac? (because then we will use the fancy DownloadObjectAndSymbolFile implementation)

I definitely agree we shouldn't crash if we see things we don't recognize, but I have no idea if it should be done this way. Greg should review this, as he's more familiar with the code.

The test looks well written. I've added a couple of suggestions you can consider including.

thanks

I think Jim (or maybe Greg?) should take a look at this.

In D33213#814041, @DemiMarie wrote:

In D33213#813037, @clayborg wrote:

Before any changes are submitted, we assume you are getting a clean test suite run.

On my system, I get build failures in clang.

This is completely wrong. The --pipe argument is used to write the port that lldb-server listens on. Making --fd an alias to that will not help.

In D31172#808177, @clayborg wrote:

In D31172#808105, @labath wrote:

• confine all the changes to m_arch to the SetArchitecture method. Right now only one of the assignments to m_arch is done outside of this method, and that one can be easily converted to a call to SetArchitecture(). This should make sure the two values never get out of sync.

That would be fine too.

I've tried that, and eventually decided against it, as it would create an uncomfortable mutual recursion between SetArchitecture and SetExecutableModule.

The version with a container object for ArchSpec+Plugin combo

In D31172#808038, @clayborg wrote:

Added a suggestion to make Target::GetArchitecturePlugin lazy and to ask if the arch is supported. Most of the time we will set this once and never need to change it, even when we attach, change arch, etc. The new suggestion will allow us to use the same arch plug-in without re-fetching it. Let me know what you think as the changes are thrown out there to see what people's opinions are.

Moving the plugin to Target and other minor fixups.

In D31172#807180, @jingham wrote:

Sorry I wasn't clear. I meant that since the Target knows everything it needs to know to vend the correct Architecture plugin, you should get it from the Target, not the Process. In general, I think that the highest class in the stack that can vend a plugin is the one that should.

In D31172#806804, @jingham wrote:

This is a good addition. However, it seems to me that since you only need an ArchSpec to make one of these Architecture plugins, which plugin you get seems fully determined by the Target, not the Process. I understand that the only need for it at present is to do a Process-related task. But that task actually takes a Thread as a parameter, so the Architecture plugin doesn't need to know it's containing process to do its job. And it seems like it diminishes the plugin's future utility to have it more limited in scope than it needs to be.

What do you think?

Have you tried that this actually works? (It doesn't work for me)

Looks great, thanks for catching this.

The architecture plugin idea

It looks like this has ground to a halt, but it seems the consensus was to try the Architecture plugin idea. I'm going to hijack this revision to maintain context, and upload a new version implementing that.

In D35083#804623, @jingham wrote:

As a general practice requiring a wrapper like this for every use of a should be locked object would make the code noisy and hard to read. The only error you would be protecting against is that somebody used the entity after the lock went out of scope. You could use some kind of markup to enforce this requirement, but you also have to be pretty sloppy to make this kind of error, so I'm not sure this it worth going to great lengths to protect against.

In this limited use, I guess I don't object seriously, but don't like this as a general pattern.

Use csignal instead of signal.h

Adding ed as freebsd owner.

cool. thanks

In D35083#802400, @jingham wrote:

Ah, maybe you meant applying the thread safety annotation to Sean's solution to enforce the contract. That's an interesting solution, but makes a non-straightforward solution even more non-straightforward, so I agree this isn't the best example...

Remove unused include from auxvector.h

I would prefer the unique_lock idea also. Lamdas make the code look ugly.

I am generally happy with this, just a couple of things I noticed below:

In D33035#798925, @abhishek.aggarwal wrote:

In D33035#798885, @labath wrote:

Hm... that's a bit of a drag. I guess the SB API never ran into this problem because it always provides it's own vector-like classes (SBModuleList, SBFileSpecList, etc.). I guess the most canonical way would be to follow that example and have your own class for a list of instructions. However, that is a bit annoying, so I do see a case for making code generated by swig as an exception to the rule.

Hmm .. Is the decision about enabling exception handling only for swig generated code lie in LLDB's community's hands or we will have to ask LLVM community for this too? I can add another class to resolve this problem but I am not sure whether it is the right way to go.

In D33035#798857, @abhishek.aggarwal wrote:

Hi Pavel .. I could remove all exception handling code from source files. However, I am still wondering how to disable exception handling while providing python functions for C++ APIs of the features. Can you suggest something here? The whole problem is occurring because of the use of vector<> as an argument to one of the C++ APIs. To provide a python interface for these APIs, I am forced to include std_vector.i (please see tools/intel-features/intel-pt/interface/PTDecoder.i file) which is introducing exception handling code.

In D34945#798808, @ravitheja wrote:

With this patch, I see the extra packet to query from server is probably unnecessary. I mean the error code is still there and it should be sufficient for the client to detect an error. As long as it does not mistake it for something else it should not be a problem ? .

remove extra pid check

cool

Address review comments

looks good

+ed as freebsd owner

Adding eugene, as he wrote that piece of code. Seems reasonable at a first glance though.