[PATCH] D79796: Sketch support for generating CC1 command line from CompilerInvocation

[Daniel Grumberg via Phabricator via llvm-commits]

dang marked 3 inline comments as done.
dang added inline comments.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:137-142
+                .Case("static", llvm::Reloc::Static)
+                .Case("pic", llvm::Reloc::PIC_)
+                .Case("ropi", llvm::Reloc::ROPI)
+                .Case("rwpi", llvm::Reloc::RWPI)
+                .Case("ropi-rwpi", llvm::Reloc::ROPI_RWPI)
+                .Case("dynamic-no-pic", llvm::Reloc::DynamicNoPIC)
----------------
dexonsmith wrote:
> I wonder if it's worth creating a `.def` file for the driver enums, something like:
> ```
> #ifdef HANDLE_RELOCATION_MODEL
> HANDLE_RELOCATION_MODEL("static", llvm::Reloc::Static)
> HANDLE_RELOCATION_MODEL("pic", llvm::Reloc::PIC_)
> HANDLE_RELOCATION_MODEL("ropi", llvm::Reloc::ROPI)
> HANDLE_RELOCATION_MODEL("rwpi", llvm::Reloc::RWPI)
> HANDLE_RELOCATION_MODEL("ropi-rwpio", llvm::Reloc::ROPI_RWPI)
> HANDLE_RELOCATION_MODEL("dynamic-no-pic", llvm::Reloc::DynamicNoPIC)
> #undef HANDLE_RELOCATION_MODEL
> #endif // HANDLE_RELOCATION_MODEL
> 
> #ifdef HANDLE_DEBUG_INFO_KIND
> HANDLE_DEBUG_INFO_KIND("line-tables-only", codegenoptions::DebugLineTablesOnly)
> HANDLE_DEBUG_INFO_KIND("line-directives-only", codegenoptions::DebugDirectivesOnly)
> HANDLE_DEBUG_INFO_KIND("limited", codegenoptions::LimitedDebugInfo)
> HANDLE_DEBUG_INFO_KIND("standalone", codegenoptions::FullDebugInfo)
> #undef HANDLE_DEBUG_INFO_KIND
> #endif // HANDLE_DEBUG_INFO_KIND
> 
> // ...
> ```
> Then you can use `HANDLE_RELOCATION_MODEL` in both `normalize` and `denormalize`, rather than duplicating the table.
> 
> Maybe we can go even further. Can you expand the `Values` array from the tablegen to include this info? Or rejigger the help text to leverage `HANDLE_RELOCATION_MODEL` (maybe pass in `ValuesDefine<HANDLE_RELOCATION_MODEL>`)? The current patch adds a value table; my first suggestion leaves us even; but maybe we can get rid of one.
I think this suggestion, I can definitely do at least this to generate the necessary switch statements. I don't see why this should be a separate .def file, I can generate this from the tablegen with an extra field named `ValuesAssociatedDefinitions` or something to that effect. I'll do that now.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3647
+                                         DiagnosticsEngine &Diags) {
+#define OPTION_WITH_MARSHALLING
+#define OPTION_WITH_MARSHALLING_FLAG(PREFIX_TYPE, NAME, ID, KIND, GROUP,       \
----------------
dexonsmith wrote:
> Seems like `Options.inc` could provide this as a default definition, not sure if that seems error-prone?
Not sure how much of this do you mean? `OPTION_WITH_MARSHALLING` is a convenience thing that forces users to opt into getting the marshalling definitions. I think it might be better to provide default empty definitions for `OPTION_WITH_MARSHALLING_FLAG` and friends to achieve a similar effect without needing `OPTION_WITH_MARSHALLING`. I you mean the actual definitions here they rely on the `ArgList &` to be named Args which might make it error prone to include these as defaults.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3665-3667
+#undef OPTION_WITH_MARSHALLING_STRING
+#undef OPTION_WITH_MARSHALLING_FLAG
+#undef OPTION_WITH_MARSHALLING
----------------
dexonsmith wrote:
> I prefer the style where the `.inc` file is responsible for the `#undef` calls. It avoids having to duplicate it everywhere (and the risk of forgetting it). WDYT?
I prefer it too, but the current tablegen backend doesn't generate them for other macros it defines, so I wanted to make it consistent... I could change the existing backend to work that way but I would then need to track all the usages of this across all the llvm-projects which I don't fancy doing. Let me know if you thing it is fine to break with the existing style for this one.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79796/new/

https://reviews.llvm.org/D79796