This is an archive of the discontinued LLVM Phabricator instance.

[llvm-nm] Include the text "@FILE" in the output of --help
ClosedPublic

Authored by mstorsjo on Oct 10 2018, 2:19 AM.

Details

Summary

libtool requires this text to be present, in order to conclude that the tool supports response files.

Diff Detail

Event Timeline

mstorsjo created this revision.Oct 10 2018, 2:19 AM

I'm not sure @FILE syntax is actually supported by llvm-nm. At least, I can't find any unit tests that test it, and @ is not present in llvm-nm.cpp. I think that change should come first?

I'm not sure @FILE syntax is actually supported by llvm-nm. At least, I can't find any unit tests that test it, and @ is not present in llvm-nm.cpp. I think that change should come first?

It does support it just fine right now (even if there's no explicit unit test for it). llvm-nm.cpp calls ParseCommandLineOptions, which internally calls ExpandResponseFiles, so this is implicit in more or less every llvm tool.

rnk accepted this revision.Oct 10 2018, 1:00 PM

lgtm

This revision is now accepted and ready to land.Oct 10 2018, 1:00 PM

I'm not sure @FILE syntax is actually supported by llvm-nm. At least, I can't find any unit tests that test it, and @ is not present in llvm-nm.cpp. I think that change should come first?

It does support it just fine right now (even if there's no explicit unit test for it). llvm-nm.cpp calls ParseCommandLineOptions, which internally calls ExpandResponseFiles, so this is implicit in more or less every llvm tool.

I see. Would you mind adding a simple test that llvm-nm actually handles @FILE as a resource file and not as a file? Not all tools use ParseCommandLineOptions, so it's not unreasonable that someone may need to change llvm-nm to avoid it for some reason.

mstorsjo updated this revision to Diff 169082.Oct 10 2018, 1:41 PM

Added a testcase that actually uses a response file. Will commit later with @rnk's approval unless there's further comments.

rupprecht accepted this revision.Oct 10 2018, 2:08 PM

lgtm, thanks for the test!

This revision was automatically updated to reflect the committed changes.