Page MenuHomePhabricator

Fixed use of -o and -k in LLDB under Windows when statically compiled with vcruntime.
Needs ReviewPublic

Authored by PatriosTheGreat on Jun 16 2021, 12:11 PM.

Details

Reviewers
teemperor
jarin
Summary

Right now if the LLDB is compiled under the windows with static vcruntime library, the -o and -k commands will not work.

The problem is that the LLDB create FILE* in lldb.exe and pass it to liblldb.dll which is an object from CRT.
Since the CRT is statically linked each of these module has its own copy of the CRT with it's own global state and the LLDB should not share CRT objects between them.

In this change I moved the logic of creating FILE* out of commands stream from Driver class to SBDebugger.
To do this I added new method: SBError SBDebugger::SetInputStream(SBStream &stream)

Command to build the LLDB:
cmake -G Ninja -DLLVM_ENABLE_PROJECTS="clang;lldb;libcxx" -DLLVM_USE_CRT_RELEASE="MT" -DLLVM_USE_CRT_MINSIZEREL="MT" -DLLVM_USE_CRT_RELWITHDEBINFO="MT" -DP
YTHON_HOME:FILEPATH=C:/Python38 -DCMAKE_C_COMPILER:STRING=cl.exe -DCMAKE_CXX_COMPILER:STRING=cl.exe ../llvm

Command which will fail:
lldb.exe -o help

See discord discussion for more details: https://discord.com/channels/636084430946959380/636732809708306432/854629125398724628
This revision is for the further discussion.

Diff Detail

Event Timeline

PatriosTheGreat requested review of this revision.Jun 16 2021, 12:11 PM
PatriosTheGreat created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2021, 12:11 PM
teemperor requested changes to this revision.Jun 21 2021, 3:47 AM

Could you move this function into the Debugger class and just make SBDebugger forward to that function? We usually keep the SB* classes as thin wrappers that only contain binding specific logic and have the actual implementation in the sibling class without the SB prefix.

The only other thing that's missing is adding this new function to the ./lldb/bindings/interface/SBDebugger.i interface file (which SWIG is using to generate Python bindings) and add a /API/ test for it. It's enough to pipe something like version\nhelp\n into the self.dbg instance and check the output (and then try with a default-constructed SBStream too).

Beside that this looks fine. Thanks for fixing this, that is a really nasty bug. I'll make a patch that adds a notice to all the existing fd/FILE based SB APIs so that the next person that will be hit by this doesn't have to track this down to this wonky behaviour.

lldb/source/API/SBDebugger.cpp
348

I think this is unspecified behaviour for a default constructed SBStream, so please make an early-exit at the top when !stream.IsValid()

378

no {} because it's just one line.

This revision now requires changes to proceed.Jun 21 2021, 3:47 AM
shafik added a subscriber: shafik.Jun 21 2021, 3:13 PM
shafik added inline comments.
lldb/source/API/SBDebugger.cpp
376

What case is this catching?

PatriosTheGreat updated this revision to Diff 353875.EditedJun 23 2021, 1:31 AM

Hi everyone.
Thanks for review.
In order to move new method code to Debugger class I also had to move there SetInputFile(lldb::FileSP file) method.
After I did that -- the reproducer tests started to fail.
If I understand correctly there is a problem to deserialize SBStream object during the reply session. In previous patch the replay tests pass since I was calling SetInputFile API method from SetInputStream.
To fix that in this patch I replaced SetInputStream method with SetInputData (const char* data, size_t size); which takes a raw chat array instead of SBStream.

PatriosTheGreat marked 3 inline comments as done.Jun 23 2021, 1:32 AM