This is an archive of the discontinued LLVM Phabricator instance.

[Driver] Escape the program path for -frecord-command-line
ClosedPublic

Authored by ravi-ramaseshan on Feb 18 2020, 6:19 PM.

Details

Summary

Similar to the rest of the command line that is recorded, the program
path must also have spaces and backslashes escaped. Without this
parsing the recorded command line becomes hard on platforms like
Windows where spaces and backslashes are common.

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptFeb 18 2020, 6:19 PM

This LGTM, but could you add a simple test with a command-line that needs the escaping?

Add a simple test with a command-line that needs the escaping.

This LGTM, but could you add a simple test with a command-line that needs the escaping?

Done.

scott.linder accepted this revision.Feb 19 2020, 2:12 PM

LGTM, thanks!

This revision is now accepted and ready to land.Feb 19 2020, 2:12 PM

Can you please land this change for me since I do not have commit rights. Thanks!

This revision was automatically updated to reflect the committed changes.

Multiple build bots were failing with the patch applied, see e.g. http://lab.llvm.org:8011/builders/llvm-clang-win-x-aarch64/builds/4890/steps/test-check-clang/logs/FAIL%3A%20Clang%3A%3Aclang_f_opts.c

Seems like on platforms with a backslash for a directory separator you get "\\\\with\\ spaces\\\\clang" rather than "/with\\ spaces/clang", although I'm not completely sure that is what we want? I imagine platforms with \ as a separator have different escaping rules?

I reverted the commit for now; @ravi-ramaseshan is it OK if I just amend the test to look for "-record-command-line" "{{.+}}with\\ spaces{{.+}}" and we can worry about whether the other escaping is correct separately?

ravi-ramaseshan added a comment.EditedFeb 20 2020, 4:26 PM

Multiple build bots were failing with the patch applied, see e.g. http://lab.llvm.org:8011/builders/llvm-clang-win-x-aarch64/builds/4890/steps/test-check-clang/logs/FAIL%3A%20Clang%3A%3Aclang_f_opts.c

Seems like on platforms with a backslash for a directory separator you get "\\\\with\\ spaces\\\\clang" rather than "/with\\ spaces/clang", although I'm not completely sure that is what we want? I imagine platforms with \ as a separator have different escaping rules?

I reverted the commit for now; @ravi-ramaseshan is it OK if I just amend the test to look for "-record-command-line" "{{.+}}with\\ spaces{{.+}}" and we can worry about whether the other escaping is correct separately?

Hi Scott, thanks for checking. Sorry about not testing it on those platforms. I think "\\\\with\\ spaces\\\\clang" looks correct. That's what we would expect on those platforms. We could use "{{.+}}{{/|\\\\\\\\}}with\\ spaces{{/|\\\\\\\\}}clang{{.+}}" to be more deliberate that this is what we want.

I think amending the commit as you did looks good to me. Will you be able to amend the commit for this or should I post another review with the fix? I verified that on Windows your suggestion works.

Sorry for the delay; I can just commit this, no need to update the review. I will hopefully be back to a terminal to push it a bit later tonight.

Recommitted as 340feac6721e840f88c1e77dd79931eea5eaccf3, hopefully this version sticks, cheers!