This is an archive of the discontinued LLVM Phabricator instance.

Handle --plugin-opt= options as alias options.
ClosedPublic

Authored by ruiu on May 21 2018, 4:13 PM.

Details

Summary

Previously, we had a loop to iterate over options starting with
--plugin-opt= and parse them by hand. But we can make OptTable
do that job for us.

Event Timeline

ruiu created this revision.May 21 2018, 4:13 PM
pcc added a comment.May 21 2018, 4:18 PM

I think these options can also be spelt -plugin-opt foo as well as -plugin-opt=foo. Can you see a good way to handle that?

ruiu added a comment.May 21 2018, 4:19 PM

Sure, I think I can just use Eq instead of J, but I'd like to do that in a follow-up patch because the options without = are not handled now.

pcc added a comment.May 21 2018, 4:24 PM

Sure, I think I can just use Eq instead of J, but I'd like to do that in a follow-up patch because the options without = are not handled now.

Are you sure? It certainly looks like they are handled, e.g.

$ ra/bin/ld.lld -plugin-opt Of
ra/bin/ld.lld: error: -plugin-opt=Of: number expected, but got 'f'
ra/bin/ld.lld: error: target emulation unknown: -m or at least one .o file required
ruiu added a comment.May 21 2018, 4:28 PM

Ouch. I don't know if there's a good way to handle options in that style. One way of doing it is to iterate over argv before passing it to OptParser to concatenate --plugin-opt and the following option with =. It doesn't feel that bad, but I'm not very sure if that's a good thing to do.

pcc added a comment.May 21 2018, 4:47 PM

Ouch. I don't know if there's a good way to handle options in that style. One way of doing it is to iterate over argv before passing it to OptParser to concatenate --plugin-opt and the following option with =. It doesn't feel that bad, but I'm not very sure if that's a good thing to do.

Yeah that doesn't seem great, but it's the best that I can think of as well.

ruiu updated this revision to Diff 147916.May 21 2018, 5:04 PM
  • concat --plugin-opt and the following argument
pcc added inline comments.May 21 2018, 5:27 PM
lld/ELF/DriverUtils.cpp
106

I don't think this correctly handles the case where the last two arguments are -plugin-opt foo. Won't you end up with an out of bounds access?

ruiu updated this revision to Diff 147935.May 21 2018, 7:25 PM
  • fix the bug
pcc accepted this revision.May 21 2018, 7:49 PM

LGTM

This revision is now accepted and ready to land.May 21 2018, 7:49 PM
This revision was automatically updated to reflect the committed changes.