This is an archive of the discontinued LLVM Phabricator instance.

[clang][Tooling] Use Windows command lines on all Windows, except Cygwin
ClosedPublic

Authored by jeremyd2019 on Oct 5 2021, 4:45 PM.

Details

Summary

Previously it only used Windows command lines for MSVC triples, but this was causing issues for windows-gnu. In fact, everything 'native' Windows (ie, not Cygwin) should use Windows command line parsing.

Diff Detail

Event Timeline

jeremyd2019 requested review of this revision.Oct 5 2021, 4:45 PM
jeremyd2019 created this revision.

Note that I found this due to a report of a crash in clangd, but I don't actually use that so just looked through the code for what was messing with paths. Users on that bug report confirmed that this fixed their issue, but there's probably still a crash somewhere on "bad" input.

Also, I don't have push access so someone would need to push this for me assuming it is approved.

nridge added a subscriber: nridge.Oct 5 2021, 6:50 PM
mstorsjo added a subscriber: rnk.Oct 5 2021, 11:36 PM

This looks ok to me I guess. Technically in this case, this is pretty much equivalent to #ifdef _WIN32, i.e. any form when running on windows, where the process considers itself windows (cygwin doesn't define _WIN32), but I guess keeping it based on getProcessTriple() is fine too.

I'll leave it open for discussion for a while, but if there's no further opinions I can accept it later.

@rnk Do you happen to have any experience here?

rnk added a comment.Oct 6 2021, 9:47 AM

This looks ok to me I guess. Technically in this case, this is pretty much equivalent to #ifdef _WIN32, i.e. any form when running on windows, where the process considers itself windows (cygwin doesn't define _WIN32), but I guess keeping it based on getProcessTriple() is fine too.

I'll leave it open for discussion for a while, but if there's no further opinions I can accept it later.

@rnk Do you happen to have any experience here?

I think the largest producer of JSON compilation databases with untokenized command line strings is CMake. If CMake uses Windows quoting rules to build up command lines, then clang should make the same assumption.

clang/lib/Tooling/JSONCompilationDatabase.cpp
138–144

This seems like it can be simplified to:

Syntax = (Triple.isOSWindows() && !Triple.isWindowsCygwinEnvironment()) ?
          JSONCommandLineSyntax::Windows : JSONCommandLineSyntax::Gnu;

Alternatively, the ifdef _WIN32 would work.

Switched to #ifdef _WIN32 instead of checking triple

jeremyd2019 marked an inline comment as done.Oct 6 2021, 10:14 AM
This revision is now accepted and ready to land.Oct 13 2021, 12:07 PM
This revision was landed with ongoing or failed builds.Oct 13 2021, 12:56 PM
This revision was automatically updated to reflect the committed changes.