Page MenuHomePhabricator

[ThinLTO] Escape module paths when printing
ClosedPublic

Authored by chrisjackson on Jul 9 2018, 10:45 AM.

Details

Summary

We have located a bug in AssemblyWriter::printModuleSummaryIndex(). This
function outputs path strings incorrectly. Backslashes in the string are not
correctly escaped. Hence, only windows environments are affected.

Consequently, if a path name contains a backslash followed by two hexadecimal
characters, the sequence is incorrectly interpreted when the output is read by
another component. This mangles the path and results in error.

This patch fixes this issue by calling printEscapedString() to output the paths.

Diff Detail

Repository
rL LLVM

Event Timeline

chrisjackson created this revision.Jul 9 2018, 10:45 AM
zturner added a reviewer: pcc.Jul 9 2018, 11:02 AM
pcc added a comment.Jul 9 2018, 11:24 AM

Would it be possible to make the test case simpler? I'm thinking that you could start with an input file with an embedded path containing a backslash, pipe it through llvm-as | llvm-dis and check that the escaping is preserved. That would mean that it could run on non-Windows as well.

dexonsmith requested changes to this revision.Jul 9 2018, 11:24 AM
dexonsmith added inline comments.
test/Assembler/asm-path-writer.ll
1 ↗(On Diff #154637)

It's unfortunate that this requires Windows. Is it possible to check in textual IR file where a path: has (properly escaped) backslashes in it, and round-trip it back to assembly?

This revision now requires changes to proceed.Jul 9 2018, 11:24 AM
tejohnson added inline comments.Jul 9 2018, 2:02 PM
test/Assembler/asm-path-writer.ll
1 ↗(On Diff #154637)

Right, this is possible since the asm parsing support is also in now. All you should need is a module path and at least one corresponding GV entry. If you use the guid form instead of a name for the gv, it will not require any corresponding IR. I.e. it can just be something like:

^0 = module: (path: "someescapedpath", hash: (0, 0, 0, 0, 0))
^1 = gv: (guid: 17407585008595848568, summaries: (function: (module: ^0, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 0), insts: 1)))

(but using an escaped path reproducing the issue)

Then just round trip through llvm-as and llvm-dis and check the output as Peter and Duncan suggested.

Followed suggestions to make the test simpler.

dexonsmith accepted this revision.Jul 10 2018, 9:00 AM

LGTM too.

This revision is now accepted and ready to land.Jul 10 2018, 9:00 AM
Closed by commit rL336908: [ThinLTO] Escape module paths when printing (authored by anng, committed by ). · Explain WhyJul 12 2018, 7:45 AM
This revision was automatically updated to reflect the committed changes.