Page MenuHomePhabricator

[Support] CommandLine API -- Allow creating custom parsers for fundamental types
Needs RevisionPublic

Authored by ckristo on Jan 2 2018, 11:52 AM.

Details

Summary

I lately stumbled upon the bug #25104 while I wanted to create a custom parser for a cl::opt<float> command line option. Since cl::parser<T> implementations were marked final in 3.7, it is no longer possible to derive from them as described in the CommandLine documentation. Deriving from cl::basic_parser<T> for fundamental types is not possible either, causing a compile error like

llvm/include/llvm/Support/CommandLine.h:1109:20: No viable conversion from 'const (anonymous namespace)::CustomParser' to 'const parser<T>'

This small patch allows to derive from cl::basic_parser<T> for fundamental types. Please note that this patch does not allow to build upon existing parsers again (as prev. possible by inheriting from cl::parser<T>), i.e. custom parser for numeric options must reimplement number parsing. This is not ideal, I know, but I do not have an idea how to make this easily possible again without removing final from cl::parser<T>. Any ideas that can be incorporated in this patch are welcome of course :-)

I kindly ask for review.

~~~~~~~~~~
Chris

Diff Detail

Event Timeline

ckristo created this revision.Jan 2 2018, 11:52 AM
ckristo updated this revision to Diff 128441.Jan 2 2018, 11:55 AM

Accidentally uploaded the wrong diff, sorry. Now the correct diff is presented for review.

ckristo added inline comments.Jan 2 2018, 12:07 PM
include/llvm/Support/CommandLine.h
1054–1056

I'll remove these three lines in the next revision -- this slipped through code cleanup, sorry!

+1 for this patch.

I have the feeling that the virtual qualifier on printOptionDiff could be avoided, as this method does not seem to be called through base class at any point ?

ckristo updated this revision to Diff 128653.Jan 4 2018, 2:34 PM

Removed some lines that were commented out and slipped through initial code cleanup + recreated patch with correct svn diff command for more context.

ckristo marked an inline comment as done.Jan 4 2018, 2:34 PM

+1 for this patch.

Hi,

Thanks for reviewing my little patch :-)

I have the feeling that the virtual qualifier on printOptionDiff could be avoided, as this method does not seem to be called through base class at any point ?

I tried to avoid making printOptionDiff virtual, but I ended up in calling basic_parser's (dummy) method implementation instead of the concrete child class' implementation. Removing virtual might be possible when revisiting the PrintOptionDiff template methods (lines 1086ff) to always pass the parser type somehow (and making a cast to the concrete parser type before calling the parser's printOptionDiff method), but to be honest I did not want to touch the templates much because I do not really get what they're doing/needed for and which version is selected in which case. Furthermore, I thought making printOptionDiff virtual does not make much of a difference because basic_parser had virtual functions already. Anyhow, if you have more insight in the PrintOptionDiff template voodoo and see a method in making printOptionDiff non-virtual, I'd be happy if you could guide me here.

@ckristo sorry for the delay, I just realized that you missed to update the cl documentation, which is the actual point of this patch :-) Otherwise LGTM

chandlerc requested changes to this revision.Mar 26 2018, 1:17 PM

I have a feeling that the original bug was from a time where we generally thought this side of commandline infrastructure was going to become a more broadly used way of building commandline tools with rich commandline interfaces. However, in recent years the project has gravitated towards the infrastructure in the Option library which is used by both Clang and LLD.

So, I'm personally not inclined to add more complexity to the commandline parsing logic. Is there a particular need that motivated this? I understand you wanted a float option, but for what purpose? Generally, we've been successful using basic integer ratios for debugging and testing options.

This revision now requires changes to proceed.Mar 26 2018, 1:17 PM

We (= the Quarkslab firm) have two use cases for extending the parsers:

  • using floating point numbers for obfuscation rations (as in « obfuscate n % of the constants). sometimes the ratio is 0.001 or such
  • using regular expression over function names. In that case it is also convenient to be able to type the option as a regexp and not only a string.

These options are used from regular passes. Is the Option library a good alternative in that case?

We (= the Quarkslab firm) have two use cases for extending the parsers:

  • using floating point numbers for obfuscation rations (as in « obfuscate n % of the constants). sometimes the ratio is 0.001 or such
  • using regular expression over function names. In that case it is also convenient to be able to type the option as a regexp and not only a string.

Is this for debugging / tuning / development?

If so, seems reasonable to use a an integer / 1000 (or larger) for ratios.

Not sure why the regexp option wouldn't be best suited to a custom type anyways. Seems like you don't need custom parsers for fundamental types there.

These options are used from regular passes. Is the Option library a good alternative in that case?

If you expect these to be customized outside of debugging / development of the passes themselves, they should be designed as part of hte API of the pass for library consumers. Then, when constructing the pass you can parameterize it in whatever way you want using data parsed out of whatever commandline library you want?

Interesting. Historically, we built an out-of-tree compiler, so llvm::cl was the natural choice to control pass behavior. Now that we work in-tree, using llvm::Option becomes a viable option.

Going that way, this patch should be repalced by a documentation patch that no longer advocates for subtyping basic types at all.