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