This is an archive of the discontinued LLVM Phabricator instance.

Fix quoting of #pragma comment for PS4
ClosedPublic

Authored by ygao on Jul 16 2015, 2:00 PM.

Details

Summary

Hi,
This is the PS4 counterpart to r229376, which quotes the library name if the name contains space.
Can someone take a look whether this looks good to go in?

  • Gao

Diff Detail

Event Timeline

ygao updated this revision to Diff 29941.Jul 16 2015, 2:00 PM
ygao retitled this revision from to Fix quoting of #pragma comment for PS4.
ygao updated this object.
ygao added a reviewer: alexr.
ygao added subscribers: cfe-commits, mkuper.
rsmith added a subscriber: rsmith.Jul 16 2015, 2:11 PM
rsmith added inline comments.
lib/CodeGen/TargetInfo.cpp
1653–1655

What happens if the library name contains an embedded \ character?

ygao added inline comments.Jul 16 2015, 2:43 PM
lib/CodeGen/TargetInfo.cpp
1653–1655

Are you thinking of the evil case where the library name ends with a backslash and then when we expand it, it might escape the separating whitespace? When I test it, the embedded backslash character always gets converted into an escape sequence "\5C" and hence looks harmless.

rsmith added inline comments.Jul 16 2015, 2:55 PM
lib/CodeGen/TargetInfo.cpp
1653–1655

Curious. What is doing that escaping, and why is it not also escaping spaces?

ygao updated this revision to Diff 29953.Jul 16 2015, 3:11 PM

Would this be more clear as:
return (Lib.find(" ") != StringRef::npos) ? "\"" + Lib.str() + "\"" : Lib;
~Aaron

Seems like a good idea. I have a slight preference of if statement over the ternary operator if that is okay with you.

ygao added a comment.Jul 16 2015, 5:40 PM

I don't have a strong feeling about it, but it seems like this wouldn't require a static helper function given how short it is (either way).
~Aaron

Sure. I'll inline that.

lib/CodeGen/TargetInfo.cpp
1653–1655

I did some research in this, and found that on Windows, the following characters are not acceptable as part of a file name or a folder name:

List of character:
\ / : * ? " < > |
End of list

Since the PS4 development is expected to be done in a Windows host environment, the backslash problem does not really impact us.

On a cross-development environment, e.g., a Linux host environment, special characters may appear in file names, and that will pose problems for quoting. For example, if a library contains both a space and a double-quote, and one cross-compiles for a Windows target, and clang tries to quote the library name. I am not sure what would be a good way to fix that. Just don't do that?

In a little experiment, I created on Linux a file with the name being a single double-quote and then I try to access the file on Windows, the file name on Windows did not display as a single double-quote but a tilda followed by 1.

Back to your question of where the escaping takes place in clang. I did not find anything in clang that does escaping, and I am coming to believe that no escaping was done, contrary to my prior statement. In the earlier testing I was doing a "clang -cc1 -triple x86_64-pc-win32 -emit-llvm", and the PrintEscapedString() in AsmWriter.cpp is printing the metadata in escaped format, but that is different than saying that the string has been converted to an escaped format. Very sorry for making the misleading statement.

ygao updated this revision to Diff 30035.Jul 17 2015, 2:17 PM
ygao updated this revision to Diff 30036.Jul 17 2015, 2:19 PM
mkuper accepted this revision.Jul 18 2015, 10:49 PM
mkuper added a reviewer: mkuper.

LGTM

This revision is now accepted and ready to land.Jul 18 2015, 10:49 PM
This revision was automatically updated to reflect the committed changes.