[cfe-commits] [PATCH] Driver: Unify CC1Options.td and Options.td

[Matthieu Monrocq]

Le 27 janvier 2012 11:46, James Molloy <james.molloy at arm.com> a écrit :

> Hi Chandler,
>
> Thanks for the comments.
>
> > Really cool! So, I assume one of the next big things on this path is to
> lift the current serialization / deserialization / parsing logic inside of
> the Frontend into the Driver as well, making the Frontend simply accept
> option structs and interfaces? That's been a long-standing goal of mine.
> Essentially, the options shouldn't escape the driver.
>
> The way I see it, the options should be serialized and deserialized by a
> CompilerInvocation object. Whether that lives in libFrontend.a or
> libDriver.a is really moot from my POV, but yes it can be moved wholesale
> into the Driver.
>
> > The other big thing I think to simplify Tools.cpp is to come up with
> common patterns for the flags that go through a round of translation when
> going > from the 'clang' CLI to the CC1 CLI, and to specify these patterns
> in the TableGen spec for the flags. That where you see this going? I just
> want to > get a good shared picture of how this will look now, so that we
> don't have too many false starts. =]
>
> So the current patch deals with the trivial "round trip" of parsing a flag
> in the driver and emitting that *exact same flag* for the frontend. Other
> flags that are different in the 'clang' CLI to the CC1 CLI, can be dealt
> with via Aliases I'd have thought? I'm not massively worried about them.
>
> What I am worried about is keeping the default options all in one place,
> along with the [de]serialization. I'm also currently concerned about the
> lack of orthogonality between -fFOO and -fno-FOO options - having the
> negation specified separately seems to be heading for disaster where
> there's no inverse for an option. I'd like to have the OptParser deal with
> negation itself, if possible.
>
>
Yes this "negation" has always bugged me, it's incredibly error-prone and
seems like something tablegen should be able to handle automatically. I am
not so sure the current option parser could easily be tweaked to deal with
this though, but I may be pessimitisc.

-- Matthieu


> > Actually, it would be awesome to have a comprehensive (even if initially
> mechanically generated) test for all of the pure forwarding behavior, and
> lack of pure forwarding. That way each time a flag is added we know to go
> add it to one of the test cases. Is that do-able?
>
> So you're suggesting a test suite that would test whether each option is
> actually being used/acted on? I'm not sure - with the current patch we
> essentially remove most forwarding. Only a CompilerInvocation will parse
> most flags intended for cc1, so the driver need not worry about them or
> forward them. What I'm suggesting is changing Clang::constructJob() to be
> essentially:
>
>   CompilerInvocation CI;
>   CompilerInvocation::CreateFromArgs(Args, CI);
>   // Modify CI here based on stuff only the driver knows - host/target
> triple, isystem, -I etc.
>   CI.Serialize(CmdArgs);
>
> So, no forwarding and no defaults for the majority of options.
>
> > I have one request to change the way you've structured the result. I'd
> like to pull all of the pure-forwarded CC1 options down into a single file.
> What I'm imagining conceptually is the following. We have essentially two
> CLIs (and hopefully eventually a third):
>
> It'd take a bit of work to go through and find all options the driver
> isn't using itself - can I please do that as a followup patch?
>
> > If this is the direction you're thinking as well, I'd like to move the
> CC1 bits down into the CC1Options.td file, and then (likely after your
> patch) rename Options.td to something better (GCCOptions.td doesn't seem
> quite right, but not sure what is). The goal being to start separating in
> the tables the CC1 interface from the compat wrappers.
>
> Yeah, although I admit that my priority is more "making Tools.cpp and
> ToolChains.cpp manageable and not ridden with copypasta" and sorting out
> the cross-compilation story via a target database. So the argument
> translation is low on my radar (but still there).
>
> I agree with you though, totally.
>
> > I've not looked at some of the details of the patch, I'll try to get to
> that tomorrow.. The only thing that looked even a little odd was the -O
> handling -- might just want comments or something there.
>
> Yeah the reason that changed was that CC1 had special options for Os/Oz,
> and the driver handles them all via Joined<"-O">.
>
> Cheers,
>
> James
>
> From: Chandler Carruth [mailto:chandlerc at google.com]
> Sent: 27 January 2012 10:20
> To: James Molloy
> Cc: cfe-commits at cs.uiuc.edu; Joerg Sonnenberger
> Subject: Re: [PATCH] Driver: Unify CC1Options.td and Options.td
>
> On Fri, Jan 27, 2012 at 1:44 AM, James Molloy <james.molloy at arm.com>
> wrote:
> Hi,
>
> The attached patch changes the way CC1 options are handled, unifying the
> generated enums for both driver and CC1. This:
>  * Will remove duplicated CC1/Driver options, removing around 140
> duplicated options.
>  * Stops the need to add new compiler options in two places.
>  * Paves the way for the driver to construct a CompilerInvocation instead
> of manually translating options and adding defaults. When this is done it
> will reduce the code in Tools.cpp drastically.
>
> Really cool! So, I assume one of the next big things on this path is to
> lift the current serialization / deserialization / parsing logic inside of
> the Frontend into the Driver as well, making the Frontend simply accept
> option structs and interfaces? That's been a long-standing goal of mine.
> Essentially, the options shouldn't escape the driver.
>
> The other big thing I think to simplify Tools.cpp is to come up with
> common patterns for the flags that go through a round of translation when
> going from the 'clang' CLI to the CC1 CLI, and to specify these patterns in
> the TableGen spec for the flags. That where you see this going? I just want
> to get a good shared picture of how this will look now, so that we don't
> have too many false starts. =]
>
> There are only two minor functionality changes (apart from that most flags
> that were not correctly forwarded by the driver now get forwarded):
>
> It would be awesome to get some more test coverage for these...
>
> Actually, it would be awesome to have a comprehensive (even if initially
> mechanically generated) test for all of the pure forwarding behavior, and
> lack of pure forwarding. That way each time a flag is added we know to go
> add it to one of the test cases. Is that do-able?
>
>
> This is the first step in having only one place that arguments are parsed
> for Clang.
>
> The patch is large mainly because of all the flags that needed to be
> de-duplicated (and I also copied over the HelpText from CC1Options.td in
> these cases because Options.td was sorely lacking in help text).
>
> Yea, the help text is a real sore spot for the actual options we expose.
>
> I have one request to change the way you've structured the result. I'd
> like to pull all of the pure-forwarded CC1 options down into a single file.
> What I'm imagining conceptually is the following. We have essentially two
> CLIs (and hopefully eventually a third):
>
> 1) CC1's CLI -- Every functional switch has to exist at least here.
> 2) GCC-compat CLI
> 3) (eventually) cl.exe compat CLI
>
> For simplicity and sanity, I think its healthy to make CC1 be as much a
> subset of #2 (or in theory #3, but that ship sailed long ago) as possible
> just to reduce the size of the interface. The result is that CC1 has all of
> the GCC-compat options that make enough sense to use directly, and al of
> the Clang-specific options in a GCC-style formulation. The GCC-compat layer
> adds on top of CC1 all the GCC flags that have too confusing of names to
> deal with internally or don't map directly onto CC1 flag, as well as any
> logic necessary to translate into CC1. #2 will also suppress any CC1 flags
> from #1 that if exposed would break some compat aspect (I don't know what
> thes ewould be, but in theory...). Finally, the #3 layer will be
> essentially the same as #2, but with essentially everything from CC1
> suppressed, and a complete wrapper with translation logic provided.
>
> If this is the direction you're thinking as well, I'd like to move the CC1
> bits down into the CC1Options.td file, and then (likely after your patch)
> rename Options.td to something better (GCCOptions.td doesn't seem quite
> right, but not sure what is). The goal being to start separating in the
> tables the CC1 interface from the compat wrappers.
>
> Thoughts?
>
> I've not looked at some of the details of the patch, I'll try to get to
> that tomorrow.. The only thing that looked even a little odd was the -O
> handling -- might just want comments or something there.
>
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120127/c8a4b392/attachment.html>