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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
