This is an archive of the discontinued LLVM Phabricator instance.

[Flang][Driver] Enable PIC in the frontend
ClosedPublic

Authored by mnadeem on Aug 9 2022, 5:12 PM.

Details

Summary

This patch does the following:

  • Consumes the PIC flags (fPIC/fPIE/fropi/frwpi etc) in flang-new. tools::ParsePICArgs() in ToolChains/CommonArgs.cpp is used for this.
  • Adds FC1Option to "-mrelocation-model", "-pic-level", and "-pic-is-pie" command line options.
  • Adds the above options to flang/Frontend/CodeGenOptions' data structure.
  • Sets the relocation model in the target machine, and
  • Sets module flags for the respective PIC/PIE type in LLVM IR.

I have tried my best to replicate how clang does things.

Diff Detail

Event Timeline

mnadeem created this revision.Aug 9 2022, 5:12 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: jdoerfert. · View Herald Transcript
mnadeem requested review of this revision.Aug 9 2022, 5:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2022, 5:12 PM

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.

clang/include/clang/Driver/Options.td
5972–5977

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 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]
6342–6347

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?

clang/lib/Driver/ToolChains/Flang.cpp
141

I would skip "Similar to clang" - this applies most of things here.

flang/test/Driver/pic-flags.f90
1

This comment is no longer valid

13–75

Why would you replace -### with -v? Same for other RUN lines.

mnadeem added inline comments.Aug 11 2022, 12:56 PM
flang/test/Driver/pic-flags.f90
13–75

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}

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

clang/include/clang/Driver/Options.td
6342–6347

Turns out that in Clang these options are indeed LangOptions. That's a bit confusing to me, but oh well.

clang/lib/Driver/ToolChains/Flang.cpp
141

Also, I'd move this whole block to a dedicated method.

flang/include/flang/Frontend/FrontendOptions.h
290–295 ↗(On Diff #451302)
flang/lib/Frontend/CompilerInvocation.cpp
184–195

Only -fpic and -fpie are tested/supported right? Please, could you trim this accordingly? Or, alternatively, expand the test.

flang/test/Driver/pic-flags.f90
13–75

Ah, of course! Thanks, I missed that earlier.

MaskRay added inline comments.Aug 12 2022, 2:23 PM
clang/include/clang/Driver/Options.td
6342–6347

clang/lib/Frontend/InitPreprocessor.cpp defines __PIC__.. IIUC the file does not know CodeGenOptions, so LangOptions isn't a bad choice.

MaskRay added inline comments.Aug 12 2022, 2:26 PM
flang/lib/Frontend/CompilerInvocation.cpp
184

Prefer == to equals

314

Use getLastArg when you need to do both hasArg and getLastArgValue.

flang/lib/Frontend/FrontendActions.cpp
533

expand auto

flang/test/Driver/driver-help.f90
133

wrong indentation?

mnadeem updated this revision to Diff 453472.Aug 17 2022, 4:39 PM
mnadeem edited the summary of this revision. (Show Details)
mnadeem added a subscriber: chrisj.
  • Moved mrelocation_model in Option.td to be near Codegen Options and put it in a let block.
  • Moved all the options from FrontendOptions to CodeGenOptions
  • Added ENUM_CODEGENOPT pragma in CodeGenOptions.def to handle the relocation model enum
  • Transferred some code to a separate AddPicOptions() function
  • Added more tests
mnadeem marked 9 inline comments as done.Aug 17 2022, 4:42 PM
mnadeem added inline comments.
flang/lib/Frontend/CompilerInvocation.cpp
184

Using llvm::StringSwitch now

184–195

Added more tests.

flang/lib/Frontend/FrontendActions.cpp
533

Changed the data type and logic.

mnadeem marked 3 inline comments as done.Aug 17 2022, 4:43 PM
MaskRay accepted this revision.Aug 20 2022, 6:10 PM
MaskRay added inline comments.
flang/test/Driver/pic-flags.f90
69

I'd use ! CHECK-ROPI-NOT: "-pic

otherwise there is a risk that the pattern matches a temporary file named *-pic*.

This revision is now accepted and ready to land.Aug 20 2022, 6:10 PM
awarzynski accepted this revision.Aug 22 2022, 4:01 AM

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!

clang/include/clang/Driver/Options.td
5251
6342–6347

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]
flang/test/Driver/pic-flags.f90
1
mnadeem marked 3 inline comments as done.Aug 22 2022, 11:09 AM
mnadeem added inline comments.
clang/include/clang/Driver/Options.td
6342–6347

Sorry, missed that. Fixed now.

mnadeem marked an inline comment as done.Aug 22 2022, 11:10 AM
This revision was automatically updated to reflect the committed changes.
clementval added inline comments.
flang/test/Driver/pic-flags.f90
3

This test has been failing on my side for very long time. Just curious why we check for PIE-LEVEL-2 when no level is given. Where is the default given?

On my machine I have -mrelocation-model static for this run line.

awarzynski added inline comments.Jul 9 2023, 1:17 AM
flang/test/Driver/pic-flags.f90
3

This test has been failing on my side for very long time.

If this is failing for you then it should also fail on one of the Flang buildbots, right? Do you know what's different/special in your configuration?

Where is the default given?

You want to check:

clementval added inline comments.Jul 10 2023, 11:13 AM
flang/test/Driver/pic-flags.f90
3

If this is failing for you then it should also fail on one of the Flang buildbots, right? Do you know what's different/special in your configuration?

Well I probably have a config that is not in any buildbot.

In my configuration the PICLevel is 0 for flang-new -v -S -emit-llvm -o - /local/home/vclement/llvm-project/flang/test/Driver/pic-flags.f90 --target=aarch64-linux-gnu 2>&1

Which makes sense since no pic option is given. I'm wondering how it default to 2 on buildbot. I'll try to investigate.