LGTM, thanks!

Hello! Thanks for the patch, generally it looks good to me, the only thing I worry about is scoped enums (please, see the comment below).

Exactly! Removed unneeded wrapper.

The problem here is that in the case of success GetCString() returns nullptr, and we fail on concatenation with None even if the expression was evaluated successfully.

Thanks! Fixed. The only thing is that GetCString() can return nullptr, which leads to None in Python, so I made a simple wrapper for that.

Hello, Pavel! Thanks for the review. Yes, this method looks like a better fit, I just didn't notice it before.

Sorry for the long delay, LGTM now, thanks!

Hello! But does the format of an exception directory for aarch64 differ completely from x86-64 one? Can we somehow adopt the existing parser to support it? If we can, I think that this check looks good (we encapsulate the difference inside PECallFrameInfo). Or may be it is worth to make a different parser for aarch64? Then I think the check would look better in ObjectFilePECOFF::CreateEHCallFrameInfo (and then we will choose a right exception directory parser depending on an architecture). What do you think on this?

Sorry, I'm OOO today, so I can't take a precise look on this now. But I'm not sure, isn't race condition possible here on the m_session_data reset?

LGTM, thank you! Can you send me an example of binary on which unwinding fails with a crash, please? It is a very interesting case...

@stella.stamenova I've fixed it with r374888, thanks again for pointing to that. The fix of unwind can fix some stepping issues, because stepping logic uses call stack information actively.

In D68980#1709327, @labath wrote:

@aleksandr.urakov, since you probably know more about windows and these tests than I do, I'd be interested in hearing your thoughts too...

Sorry, I didn't see Pavel's message. In this case I have no objections to the patch if other people also won't have.

LGTM!

In D67347#1705563, @mstorsjo wrote:

Quick question here; will unwinding using DWARF debug info still work like before after this, for binaries that don't use SEH for exception unwinding?

Thanks a lot for the review!

Hi! Could you choose some other names for com1.o and com2.o, please? :) They are reserved names on Windows, so there are problems with checking out this change.

Hello! I am sorry for a delay with reply, I was OOO. Thanks all for the review!

In D68258#1690756, @aleksandr.urakov wrote:

I've made it in the way similar to Zachary have made for the SymbolFileNativePDB plugin. An environment variable could be more convenient e.g. to run a bunch of tests using the lldb-test option.

I've made it in the way similar to Zachary have made for the SymbolFileNativePDB plugin. An environment variable could be more convenient e.g. to run a bunch of tests using the lldb-test option.

Thank you! I've added a switch in a manner similar to one that Zachary has chosen for SymbolFilePDB/NativePDB plugins here: D68258

Gentle ping! What do you think of this?

In D67954#1680536, @labath wrote:

Have you considered going the "native" route directly? My understanding is that this route is already functional on x86 (modulo watchpoints, which I need to get around to reviewing). It would be great to be able to delete the in-process route soon, as it's not good to have both for a long time, and for that we need to ensure that the lldb-server route does not lag behind in features. I'm not really sure what's needed to enable the lldb-server mechanism, but @aleksandr.urakov should.

In D67222#1680613, @labath wrote:

Btw, in the context of D67954, I realized that I don't know how to actually enable the usage of lldb-server in lldb on windows. Can you share how you did that ?

Update the patch due to Pavel's request.

Hi Jason, thanks for the review!

Update due to the requests.

Thanks all for taking a look!

Update diff due to Pavel's requests.

It's a good point, thank you! I had the same thoughts when I done it, but I'm not completely sure. The problem is that an object file can't be completely responsible for choosing an unwind plan, because some plans are produced by symbol files (and an object file knows nothing about that, if I understand right). So we can move only a part of the plan-choosing-functionality from FuncUnwinders to ObjectFiles, but will it be better? In that case both FuncUnwinders and ObjectFiles will be responsible for managing plans, won't it add an additional complexity to the solution?

Thanks all!

Determine whether 8-byte watchpoints are supported or not statically.

Check bitness of the target when checking the watchpoint size.

Address the requested changes.

Address the requested changes.

LGTM, thanks!

LGTM!

That makes sense, thank you!

LGTM, thanks! Sorry for the long delay with the reply, I had days off.

LGTM! So the other functions (e.g. GetOrCreateDeclContextForUid, GetParentDeclContext etc.) will be also replaced in the future in a corresponding manner?

I do not fully understand how this works... Does the second fgets sets error to EINTR?

LGTM!

LGTM, thanks!

Ah, yes, sure, it will just be moved on the lldb-server side. Thanks again for the clarification!

In D53753#1504589, @labath wrote:

When we evaluate an expression, we jit a bunch of opcodes into the inferior memory and then have it execute them. For that to work, we need to allocate some memory in order to store the opcodes. We cannot use the general expression engine to jit that expression, as we would be back to square one, so we manually set the PC to the entry point of the mmap function, and set the argument values as if it was being called. Then we just let the inferior loose and have it allocate the memory for us and return it. For this to work, we need abi knowledge both to correctly set the arguments of mmap, and to retrieve its result.

In D53753#1504361, @labath wrote:

I'm not sure what exactly are the consequences of not using the correct ABI definition here. I think the case where the difference may start to become obvious is if you try to get argument values of a function for which you don't have debug info for.

It sounds strange to me... If we don't have symbols for a function, then we can't even know amount of its arguments, so how can we retrieve them? Also e.g. on Windows x86 both stdcall, ccall, thiscall and fastcall are commonly used, so it would be strange to use some "default" ABI...

@lanza Hello! AFAIU, these values have nothing to do with the Microsoft x64 calling convention. They are used by ABISysV_x86_64, which specifies ABI for working with code injectable into a process being debugged. It requires six arguments to be passed through registers (see GetArgumentValues).

LGTM, thanks!

Thanks for the review! Sorry, I've forgot to include context in the updated patch. Yes, it's exactly the case that is broken, I've left FIXME there until it will be fixed.

Thank you for the comments, I agree with you. I've fixed the patch.

Sorry for the long delay with the response, I had days off. LGTM, thanks!