Be more strict when checking the -flto option value
ClosedPublic

Authored by yamaguchi on Jun 9 2017, 8:16 AM.

Details

Summary

It seems -flto must be either "thin" or "full". I think the use of
containValue is just a typo.

Diff Detail

Repository
rL LLVM
yamaguchi created this revision.Jun 9 2017, 8:16 AM
tejohnson edited subscribers, added: cfe-commits; removed: llvm-commits.

Make sure you add cfe-commits to the mailing list for clang changes (and llvm-commits for llvm changes, I accidentally added that one first and then fixed it), since all patches should go to the full mailing list.

This fix looks correct to me. Please add a test case though. Perhaps there should be an error if the value is something other than "thin" or "lto"? E.g. see the error that will get emitted if the value is not one of those in clang/lib/Driver/Driver.cpp.

yamaguchi abandoned this revision.Jun 9 2017, 9:35 AM
yamaguchi reclaimed this revision.
yamaguchi updated this revision to Diff 102051.Jun 9 2017, 10:27 AM

I think we don't need additional testcase, because this is non-functional change. Driver.cpp will emit error if value was not "thin" nor "full". This testcase is at clang/test/CodeGen/thinlto-backend-option.ll.

I think we don't need additional testcase, because this is non-functional change. Driver.cpp will emit error if value was not "thin" nor "full". This testcase is at clang/test/CodeGen/thinlto-backend-option.ll.

That test case is checking that -flto=thin is accepted by the driver. I don't see it checking for an unknown -flto= value. But in any case, you can go through the code you are fixing here without hitting the driver check by invoking clang -cc1 (%clang_cc1 in the test case). I just confirmed that -flto=thinfoo is flagged as an error by clang (when it goes through the driver), but is silently accepted if I pass directly to clang -cc1. So this is definitely a bug fix and it would be good to check for invalid -flto values and given an error, then have a test that uses %clang_cc1 and confirms that an unknown -flto= value gets that error.

yamaguchi updated this revision to Diff 102551.Jun 14 2017, 8:13 AM

Update patch. Add testcase and check if argument is valid or not.

tejohnson accepted this revision.Jun 14 2017, 8:19 AM

LGTM, thanks for the fix!

This revision is now accepted and ready to land.Jun 14 2017, 8:19 AM
This revision was automatically updated to reflect the committed changes.