Page MenuHomePhabricator

Various build fixes for lldb on MinGW
ClosedPublic

Authored by hhb on Aug 2 2019, 10:29 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

hhb created this revision.Aug 2 2019, 10:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2019, 10:29 PM
hhb added a reviewer: pirama.Aug 2 2019, 10:38 PM

As context (I haven't followed lldb almost at all), what's the general status of lldb on windows - generally working, or still a work in progress? Does it work with dwarf debug info in COFF (as commonly used in mingw environments), in addition to PDB? I.e. is the existing dwarf support tied to the ELF/MachO formats, or is it decoupled?

As context (I haven't followed lldb almost at all), what's the general status of lldb on windows - generally working, or still a work in progress? Does it work with dwarf debug info in COFF (as commonly used in mingw environments), in addition to PDB? I.e. is the existing dwarf support tied to the ELF/MachO formats, or is it decoupled?

It is decoupled. As long as the ObjectFileCOFF hands out the right section for the section enumeration in lldb-enumerations.h in the lldb::SectionType, it should all just work. The enumerations in question are all of the enums that start with eSectionTypeDWARF. It seems to have support in ObjectFilePECOFF::CreateSections(...). So I think it should all just work. Let us know if there are any issues.

xbolva00 added inline comments.
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
19 ↗(On Diff #213175)

Maybe drop usleep and use C++’s

https://en.cppreference.com/w/cpp/thread/sleep_for ?

I took @xbolva00's suggestion one step further, and removed the usleep and the related compat code in r367814. So, some of your changes are no longer needed once you rebase past that.

I have a couple of additional small comments inline, but other than that, this seems fine. I'm not sure how active is @zturner these days (@amccarth and @stella.stamenova are probably better reviewers for windows stuff now), but this seems pretty straight-forward, so I don't think we have to wait for them.

lldb/source/Initialization/SystemInitializerCommon.cpp
28 ↗(On Diff #213175)

In r346625, we deleted all/most comments like this. Probably best to not introduce new ones...

lldb/tools/driver/Platform.h
20 ↗(On Diff #213175)

It looks like we already have the HAVE_SYS_TYPES_H macro, which we can use for this purpose, so I'd use that, and move it out of the _MSC_VER block as we should really include this header on other platforms too (but we probably haven't done that because it got included transitively already...)

Yes, zturner isn't as active here as he was before.

I'm happy to review patches regarding LLDB on Windows, even though I don't know much about Mingw.

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
19 ↗(On Diff #213175)

+1. usleep is deprecated in POSIX and is essentially meaningless on Windows (in terms of how the scheduler works) unless you're waiting 1000 or more microseconds. At this point, sleep_for is likely to be better supported by the platforms.

lldb/tools/lldb-vscode/VSCode.cpp
19 ↗(On Diff #213175)

I would suggest moving the #ifndef __MINGW32__ up a line to keep the NOMINMAX adjacent to the include of Windows.h. NOMINMAX is relevant only to Windows.h.

lldb/tools/lldb-vscode/lldb-vscode.cpp
55 ↗(On Diff #213175)

Nothing in the rest of this .cpp file uses PATH_MAX, so just delete the #define instead of executing it conditionally.

hhb updated this revision to Diff 213448.Aug 5 2019, 1:26 PM
hhb marked 4 inline comments as done.

Fix comments.

lldb/tools/lldb-vscode/lldb-vscode.cpp
55 ↗(On Diff #213175)

PATH_MAX is used in SendProcessEvent (line 283)

amccarth marked an inline comment as done.Aug 5 2019, 3:19 PM
amccarth added inline comments.
lldb/tools/lldb-vscode/lldb-vscode.cpp
55 ↗(On Diff #213175)

Got it. When I asked Phabricator to show me all the lines, it didn't show me _all_ the lines.

That said, it seems unfortunate that it's yet another fixed-length filename buffer instead of getting a ConstString from the FileSpec. But that's outside the scope of this change.

hhb updated this revision to Diff 213505.Aug 5 2019, 5:59 PM

Rebase

labath accepted this revision.Aug 5 2019, 11:00 PM
labath added inline comments.
lldb/tools/lldb-vscode/lldb-vscode.cpp
55 ↗(On Diff #213175)

The problem is that you're dealing with the SBFileSpec here, and that makes things slightly complicated (stable api, not being able to return c++ objects through it, etc...).

This revision is now accepted and ready to land.Aug 5 2019, 11:00 PM
jingham added inline comments.
lldb/tools/lldb-vscode/lldb-vscode.cpp
55 ↗(On Diff #213175)

There's no reason we couldn't add an SBString class to vend non-fixed length strings. There haven't been enough instances where we really needed it to motivate that, but we have SBStringList to wrap vectors of strings, so it wouldn't be out of place.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2019, 11:21 AM