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

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Left a few more comments and also added Diana as a reviewer. Would be good to get an extra pair of eyes on this :)



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


================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:120
 
+  // -fPIC/-fPIE and their variants. Similar to clang.
+  llvm::Reloc::Model RelocationModel;
----------------
awarzynski wrote:
> I would skip "Similar to clang" - this applies most of things here.
Also, I'd move this whole block to a dedicated method.


================
Comment at: flang/include/flang/Frontend/FrontendOptions.h:290-295
+  /// Some targets need the pic levels. These are stored as
+  /// LLVM module flags.
+  llvm::Optional<llvm::PICLevel::Level> PICLevel;
+  llvm::Optional<llvm::PIELevel::Level> PIELevel;
+  /// For setting up the target machine.
+  llvm::Optional<llvm::Reloc::Model> RelocModel;
----------------
To me these are CodeGen options. Could you move this to https://github.com/llvm/llvm-project/blob/main/flang/include/flang/Frontend/CodeGenOptions.h?


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:151-162
+  if (model.equals("static"))
+    return llvm::Reloc::Static;
+  if (model.equals("pic"))
+    return llvm::Reloc::PIC_;
+  if (model.equals("dynamic-no-pic"))
+    return llvm::Reloc::DynamicNoPIC;
+  if (model.equals("ropi"))
----------------
Only `-fpic` and `-fpie` are tested/supported right? Please, could you trim this accordingly? Or, alternatively, expand the test.


================
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
+
----------------
mnadeem wrote:
> awarzynski wrote:
> > Why would you replace `-###` with `-v`? Same for other RUN lines. 
> I needed the command to run because I added check lines for the emitted LLVM IR, for example:
>     ! CHECK-PIE-LEVEL1: !"PIC Level", i32 1}
>     ! CHECK-PIE-LEVEL1: !"PIE Level", i32 1}
Ah, of course! Thanks, I missed that earlier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131533