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

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Many thanks for implementing this! I've only skimmed through so far, but mostly makes sense. Will take a closer look later.

Could you update the summary by adding more detail? What options are enabled and whether the semantics from Clang are preserved or not (they should unless there is a good reason not to)? It would help if you could list all of them. More comments inline.



================
Comment at: clang/include/clang/Driver/Options.td:5954-5959
+def mrelocation_model : Separate<["-"], "mrelocation-model">,
+  HelpText<"The relocation model to use">, Values<"static,pic,ropi,rwpi,ropi-rwpi,dynamic-no-pic">,
+  Flags<[CC1Option, CC1AsOption, FC1Option, NoDriverOption]>,
+  NormalizedValuesScope<"llvm::Reloc">,
+  NormalizedValues<["Static", "PIC_", "ROPI", "RWPI", "ROPI_RWPI", "DynamicNoPIC"]>,
+  MarshallingInfoEnum<CodeGenOpts<"RelocationModel">, "PIC_">;
----------------
As per comments in Options.td, this is a "Code-gen Option" rather than a "Language Option". Could you move it back somewhere near the original [[ https://github.com/llvm/llvm-project/blob/e5e93b6130bde96d7e14851e218c5bf055f8a834/clang/include/clang/Driver/Options.td#L5231-L5233 | comment ]]?

I would also suggest using `let` (there's going to be more options like this):
```
let Flags = [CC1Option, CC1AsOption, FC1Option, NoDriverOption] {

def mrelocation_model : Separate<["-"], "mrelocation-model">,
  HelpText<"The relocation model to use">, Values<"static,pic,ropi,rwpi,ropi-rwpi,dynamic-no-pic">,
  NormalizedValuesScope<"llvm::Reloc">,
  NormalizedValues<["Static", "PIC_", "ROPI", "RWPI", "ROPI_RWPI", "DynamicNoPIC"]>,
  MarshallingInfoEnum<CodeGenOpts<"RelocationModel">, "PIC_">;

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


================
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">>;
----------------
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?


================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:120
 
+  // -fPIC/-fPIE and their variants. Similar to clang.
+  llvm::Reloc::Model RelocationModel;
----------------
I would skip "Similar to clang" - this applies most of things here.


================
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
 
----------------
This comment is no longer valid


================
Comment at: flang/test/Driver/pic-flags.f90:3
 
-! RUN: %flang -### %s --target=aarch64-linux-gnu 2>&1 | FileCheck %s --check-prefix=CHECK-NOPIE
-! RUN: %flang -### %s --target=aarch64-linux-gnu -fno-pie 2>&1 | FileCheck %s --check-prefix=CHECK-NOPIE
-
-! RUN: %flang -### %s --target=aarch64-linux-gnu -fpie 2>&1 | FileCheck %s --check-prefix=CHECK-PIE
-
-! CHECK-NOPIE: "-fc1"
-! CHECk-NOPIE-NOT: "-fpic"
-! CHECK-NOPIE: "{{.*}}ld"
-! CHECK-NOPIE-NOT: "-pie"
-
-! CHECK-PIE: "-fc1"
-!! TODO Once Flang supports `-fpie`, it //should// use -fpic when invoking `flang -fc1`. Update the following line once `-fpie` is
-! available.
-! CHECk-PIE-NOT: "-fpic"
-! CHECK-PIE: "{{.*}}ld"
-! CHECK-PIE-NOT: "-pie"
+! RUN: %flang -v -S -emit-llvm -o - %s --target=aarch64-linux-gnu -fno-pie 2>&1 | FileCheck %s --check-prefix=CHECK-NOPIE
+
----------------
Why would you replace `-###` with `-v`? Same for other RUN lines. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131533