This is an archive of the discontinued LLVM Phabricator instance.

Differential D6792

[COFF] Don't try to add quotes to already quoted linker directives
ClosedPublic

Authored by mkuper on Dec 29 2014, 2:01 AM.

Details

Reviewers
rnk
Commits
rGc43b063358a8: [COFF] Don't try to add quotes to already quoted linker directives
rL224998: [COFF] Don't try to add quotes to already quoted linker directives
Summary

If a linker directive is already quoted, don't try to quote it again, otherwise it creates a mess.

This pops up in places like:
#pragma comment(linker,"\"/foo bar'\"")

Diff Detail

Event Timeline

mkuper updated this revision to Diff 17666.Dec 29 2014, 2:01 AM
mkuper retitled this revision from to [COFF] Don't try to add quotes to already quoted linker directives.
mkuper updated this object.
mkuper edited the test plan for this revision. (Show Details)
mkuper added a reviewer: rnk.
mkuper added a subscriber: Unknown Object (MLST).
majnemer added a subscriber: majnemer.Dec 29 2014, 2:13 AM
majnemer added inline comments.
lib/CodeGen/TargetLoweringObjectFileImpl.cpp
989

This would probably be a little nicer as:
if (!Op.startswith("\"") && Op.find(" ") != StringRef::npos)

This way we don't need to hunt for spaces if it starts with a \" sequence.

mkuper added inline comments.Dec 29 2014, 3:32 AM
lib/CodeGen/TargetLoweringObjectFileImpl.cpp
989

Right, noticed that myself after uploading, thanks. :-)

rnk accepted this revision.Dec 30 2014, 9:57 AM
rnk edited edge metadata.

lgtm with David's suggestion

This revision is now accepted and ready to land.Dec 30 2014, 9:57 AM
Closed by commit rL224998: [COFF] Don't try to add quotes to already quoted linker directives (authored by mkuper). · Explain WhyDec 30 2014, 11:24 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lib/
CodeGen/
TargetLoweringObjectFileImpl.cpp
TargetLoweringObjectFileImpl.cpp (revision 224899)
2 lines
test/
MC/
COFF/
linker-options.ll
linker-options.ll (revision 224899)
3 lines

Diff 17666

lib/CodeGen/TargetLoweringObjectFileImpl.cpp

Show First 20 Lines • Show All 980 Lines • ▼ Show 20 LinesemitModuleFlags(MCStreamer &Streamer,
Streamer.SwitchSection(Sec); Streamer.SwitchSection(Sec);
for (unsigned i = 0, e = LinkerOptions->getNumOperands(); i != e; ++i) { for (unsigned i = 0, e = LinkerOptions->getNumOperands(); i != e; ++i) {
MDNode *MDOptions = cast<MDNode>(LinkerOptions->getOperand(i)); MDNode *MDOptions = cast<MDNode>(LinkerOptions->getOperand(i));
for (unsigned ii = 0, ie = MDOptions->getNumOperands(); ii != ie; ++ii) { for (unsigned ii = 0, ie = MDOptions->getNumOperands(); ii != ie; ++ii) {
MDString *MDOption = cast<MDString>(MDOptions->getOperand(ii)); MDString *MDOption = cast<MDString>(MDOptions->getOperand(ii));
StringRef Op = MDOption->getString(); StringRef Op = MDOption->getString();
// Lead with a space for consistency with our dllexport implementation. // Lead with a space for consistency with our dllexport implementation.
std::string Escaped(" "); std::string Escaped(" ");
if (Op.find(" ") != StringRef::npos) { if ((Op.find(" ") != StringRef::npos) && (!Op.startswith("\""))) {
majnemerUnsubmitted
Not Done
ReplyInline Actions

This would probably be a little nicer as:
if (!Op.startswith("\"") && Op.find(" ") != StringRef::npos)

This way we don't need to hunt for spaces if it starts with a \" sequence.

majnemer: This would probably be a little nicer as: `if (!Op.startswith("\"") && Op.find(" ") !=…
mkuperAuthorUnsubmitted
Not Done
ReplyInline Actions

Right, noticed that myself after uploading, thanks. :-)

mkuper: Right, noticed that myself after uploading, thanks. :-)
// The PE-COFF spec says args with spaces must be quoted. It doesn't say // The PE-COFF spec says args with spaces must be quoted. It doesn't say
// how to escape quotes, but it probably uses this algorithm: // how to escape quotes, but it probably uses this algorithm:
// http://msdn.microsoft.com/en-us/library/17w5ykft(v=vs.85).aspx // http://msdn.microsoft.com/en-us/library/17w5ykft(v=vs.85).aspx
// FIXME: Reuse escaping code from Support/Windows/Program.inc // FIXME: Reuse escaping code from Support/Windows/Program.inc
Escaped.push_back('\"'); Escaped.push_back('\"');
Escaped.append(Op); Escaped.append(Op);
Escaped.push_back('\"'); Escaped.push_back('\"');
} else { } else {
Show All 18 Lines

test/MC/COFF/linker-options.ll

; RUN: llc -O0 -mtriple=i386-pc-win32 -filetype=asm -o - %s | FileCheck %s ; RUN: llc -O0 -mtriple=i386-pc-win32 -filetype=asm -o - %s | FileCheck %s
!0 = !{i32 6, !"Linker Options", !{!{!"/DEFAULTLIB:msvcrt.lib"}, !{!"/DEFAULTLIB:msvcrt.lib", !"/DEFAULTLIB:secur32.lib"}, !{!"/DEFAULTLIB:C:\5Cpath to\5Casan_rt.lib"}, !{!"/with spaces"}}} !0 = !{i32 6, !"Linker Options", !{!{!"/DEFAULTLIB:msvcrt.lib"}, !{!"/DEFAULTLIB:msvcrt.lib", !"/DEFAULTLIB:secur32.lib"}, !{!"/DEFAULTLIB:C:\5Cpath to\5Casan_rt.lib"}, !{!"/with spaces"}, !{!"\22/quoted spaces\22"}}}
!llvm.module.flags = !{ !0 } !llvm.module.flags = !{ !0 }
define dllexport void @foo() { define dllexport void @foo() {
ret void ret void
} }
; CHECK: .section .drectve,"yn" ; CHECK: .section .drectve,"yn"
; CHECK: .ascii " /DEFAULTLIB:msvcrt.lib" ; CHECK: .ascii " /DEFAULTLIB:msvcrt.lib"
; CHECK: .ascii " /DEFAULTLIB:msvcrt.lib" ; CHECK: .ascii " /DEFAULTLIB:msvcrt.lib"
; CHECK: .ascii " /DEFAULTLIB:secur32.lib" ; CHECK: .ascii " /DEFAULTLIB:secur32.lib"
; CHECK: .ascii " \"/DEFAULTLIB:C:\\path to\\asan_rt.lib\"" ; CHECK: .ascii " \"/DEFAULTLIB:C:\\path to\\asan_rt.lib\""
; CHECK: .ascii " \"/with spaces\"" ; CHECK: .ascii " \"/with spaces\""
; CHECK: .ascii " \"/quoted spaces\""
; CHECK: .ascii " /EXPORT:_foo" ; CHECK: .ascii " /EXPORT:_foo"