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

[James Molloy]

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.

> 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.