[PATCH] D131533: [Flang][Driver] Enable PIC in the frontend

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski accepted this revision.
awarzynski added a comment.

Thanks for all the updates and for working on this! I'm not an expert in the semantics of `-fpie`/`-fpic`/`-mrelocation-model`, but this basically replicates the logic in Clang and I am not aware of any good reasons for Flang to diverge from that. This looks good to me.

I've left a few minor comments inline and would appreciate if you could address them before landing this. Thanks again for taking this on!



================
Comment at: clang/include/clang/Driver/Options.td:6320-6325
+def pic_level : Separate<["-"], "pic-level">,
+  HelpText<"Value for __PIC__">,
+  MarshallingInfoInt<LangOpts<"PICLevel">>;
+def pic_is_pie : Flag<["-"], "pic-is-pie">,
+  HelpText<"File is for a position independent executable">,
+  MarshallingInfoFlag<LangOpts<"PIE">>;
----------------
MaskRay wrote:
> awarzynski wrote:
> > awarzynski wrote:
> > > These are code-gen options to me. While originally located under "Language Options", I think that it would make more sense to move them near "CodeGen Options" instead (e.g. near `mrelocation_model`). @MaskRay any thoughts?
> > Turns out that in Clang these options are indeed [[ https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/LangOptions.def#L199 | LangOptions ]]. That's a bit confusing to me, but oh well.
> clang/lib/Frontend/InitPreprocessor.cpp defines `__PIC__`.. IIUC the file does not know CodeGenOptions, so LangOptions isn't a bad choice.
I think that we all agree that these options should remain somewhere withing the "Language Options" block, i.e. below:

```
//===----------------------------------------------------------------------===//
// Language Options
//===----------------------------------------------------------------------===//
```

As previously, you can use a `let` statement there:
```
let Flags = [CC1Option, FC1Option, NoDriverOption] in {
def pic_level : Separate<["-"], "pic-level">,
  HelpText<"Value for __PIC__">,
  MarshallingInfoInt<LangOpts<"PICLevel">>;
def pic_is_pie : Flag<["-"], "pic-is-pie">,
  HelpText<"File is for a position independent executable">,
  MarshallingInfoFlag<LangOpts<"PIE">>;

} // let Flags = [CC1Option, FC1Option, NoDriverOption]
```


================
Comment at: clang/include/clang/Driver/Options.td:5245
+
+} // let Flags = [CC1Option, CC1AsOption, NoDriverOption]
+
----------------



================
Comment at: flang/test/Driver/pic-flags.f90:1
-! Verify that in contrast to Clang, Flang does not default to generating position independent executables/code
 
----------------



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

https://reviews.llvm.org/D131533