Page MenuHomePhabricator

Fix processing of path names in response files on Windows
ClosedPublic

Authored by dmikulin on Mar 1 2018, 4:51 PM.

Details

Summary

On Windows we need to be able to process response files with Windows-style path names which include backslashes.

Diff Detail

Repository
rL LLVM

Event Timeline

dmikulin created this revision.Mar 1 2018, 4:51 PM
zturner added a subscriber: zturner.

I remember someone said they could think of some issues with doing this, but I can't remember what those issues where. +rnk and +ruiu

ruiu added a comment.Mar 1 2018, 5:05 PM

I don't think this is a right thing to do because creating a Windows executable doesn't necessarily mean that you are doing it on Command Prompt and vice versa. clang and lld support --rsp-quoting={windows,posix} to explicitly tell the driver how to parse response files. Didn't that work for you?

rnk added inline comments.Mar 1 2018, 5:07 PM
llvm/lib/Support/CommandLine.cpp
1085 ↗(On Diff #136644)

I would say Triple(sys::getProcessTriple()).isOSWindows()

llvm/unittests/Support/CommandLineTest.cpp
636 ↗(On Diff #136644)

ditto

644 ↗(On Diff #136644)

This assumes a writable cwd. I think you want this pattern:

int FileDescriptor;
SmallString<64> TempPath;
ASSERT_NO_ERROR(
    fs::createTemporaryFile("resp-", ".txt", FileDescriptor, TempPath));
rnk added a comment.Mar 1 2018, 5:08 PM

I don't think this is a right thing to do because creating a Windows executable doesn't necessarily mean that you are doing it on Command Prompt and vice versa. clang and lld support --rsp-quoting={windows,posix} to explicitly tell the driver how to parse response files. Didn't that work for you?

LLVM's tools do not have a standard way to indicate the response file quoting style.

ruiu added a comment.Mar 1 2018, 5:11 PM

LLVM's tools do not have a standard way to indicate the response file quoting style.

But if there was a problem that we had to fix by adding --rsp-quoting option, that will become a problem with this patch, no?

LLVM's tools do not have a standard way to indicate the response file quoting style.

But if there was a problem that we had to fix by adding --rsp-quoting option, that will become a problem with this patch, no?

Adding a --rsp-quoting option globally would likely change the same place this does.

I don't think this is a right thing to do because creating a Windows executable doesn't necessarily mean that you are doing it on Command Prompt and vice versa. clang and lld support --rsp-quoting={windows,posix} to explicitly tell the driver how to parse response files. Didn't that work for you?

But we still have to choose a default. Why is using GNU parsing on Windows the right thing to do?

rnk added a comment.Mar 2 2018, 12:58 PM

I think this is the right default for Windows. We can add an --rsp-quoting argument as a follow-up if someone needs it.

ruiu added a comment.Mar 2 2018, 1:26 PM

My memory is vague -- what was the issue that we needed to add --rsp-quoting in the first place?

dmikulin updated this revision to Diff 137024.Mar 5 2018, 9:34 AM

Addressed review comments.

ruiu added a comment.Mar 5 2018, 10:23 AM

I don't want to block this, and it sounds reasonable to make Windows command line grammar default on Windows. I'll defer someone who is more familiar with this code than me to give a LGTM.

rnk accepted this revision.Mar 5 2018, 10:37 AM

OK, ship it. :)

This revision is now accepted and ready to land.Mar 5 2018, 10:37 AM
This revision was automatically updated to reflect the committed changes.