[ClangFE] Add support for option -mno-pic-data-is-text-relative

[jonpa]

Summary

(Patch in progress)

Add support for this GCC option which has the purpose of disallowing text relative accesses of module local symbols with PIC.

I am fumbling a bit with this and would like to ask for some help as I have no good idea of how this is best done. My idea so far has been to

• patch adjustGVALinkageForAttributes() to change the Linkage to External for these GlobalValues.

• Add Driver option but I could not make this work all the way into CompilerInvocation.cpp. The round-trip check of the arguments is failing... :-/

[MaskRay]

Can you describe the motivation adding this option? Is it for a FDPIC ABI? Removing the assumption that text and data has a constant offset generally make the code worse for data access. This is almost assuredly a bad idea if the system has a memory management unit.

[uweigand]

In D137044#3901025, @MaskRay wrote:

Can you describe the motivation adding this option? Is it for a FDPIC ABI? Removing the assumption that text and data has a constant offset generally make the code worse for data access. This is almost assuredly a bad idea if the system has a memory management unit.

This is intended to be used for "kpatch" - the Linux kernel live-patching facility. This allows replacing parts of the kernel code with an updated version at runtime. However, the *data* segment cannot be moved since it is already in use (and pointers to the data may be around), and so the replacement text segment will have to refer to the original data segment. Since the replacement text segment will generally end up at different addresses than the original one, this means the compiler cannot make assumptions about the relative distance from code to data.

Of course this in general results in worse code, but that is considered an acceptable cost to enable the live-patching feature. Note that GCC already supports the same option with the same semantics, used for the same purpose. The intent of supporting it in clang as well is simply to optionally enabling building the Linux kernel with clang instead of GCC while preserving the full set of kernel features.

[MaskRay]

In D137044#3901481, @uweigand wrote:

In D137044#3901025, @MaskRay wrote:

Can you describe the motivation adding this option? Is it for a FDPIC ABI? Removing the assumption that text and data has a constant offset generally make the code worse for data access. This is almost assuredly a bad idea if the system has a memory management unit.

This is intended to be used for "kpatch" - the Linux kernel live-patching facility. This allows replacing parts of the kernel code with an updated version at runtime. However, the *data* segment cannot be moved since it is already in use (and pointers to the data may be around), and so the replacement text segment will have to refer to the original data segment. Since the replacement text segment will generally end up at different addresses than the original one, this means the compiler cannot make assumptions about the relative distance from code to data.

Of course this in general results in worse code, but that is considered an acceptable cost to enable the live-patching feature. Note that GCC already supports the same option with the same semantics, used for the same purpose. The intent of supporting it in clang as well is simply to optionally enabling building the Linux kernel with clang instead of GCC while preserving the full set of kernel features.

I know the existence of livepatch in the Linux kernel though unfamiliar with it. The kernel doesn't use -mno-pic-data-is-text-relative right now for any port. Why does SystemZ need -mno-pic-data-is-text-relative for livepatch?

[MaskRay]

For work-in-progress patches, there is a convention to use [WIP] or a similar tag. [ClangFE] has never been used for Clang patches.
If you want to ask how to implement this, you may raise a thread on discourse. I think quite a lot of places need to be changed to support -mno-pic-data-is-text-relative.

[uweigand]

In D137044#3906898, @MaskRay wrote:

In D137044#3901481, @uweigand wrote:

In D137044#3901025, @MaskRay wrote:

Can you describe the motivation adding this option? Is it for a FDPIC ABI? Removing the assumption that text and data has a constant offset generally make the code worse for data access. This is almost assuredly a bad idea if the system has a memory management unit.

This is intended to be used for "kpatch" - the Linux kernel live-patching facility. This allows replacing parts of the kernel code with an updated version at runtime. However, the *data* segment cannot be moved since it is already in use (and pointers to the data may be around), and so the replacement text segment will have to refer to the original data segment. Since the replacement text segment will generally end up at different addresses than the original one, this means the compiler cannot make assumptions about the relative distance from code to data.

Of course this in general results in worse code, but that is considered an acceptable cost to enable the live-patching feature. Note that GCC already supports the same option with the same semantics, used for the same purpose. The intent of supporting it in clang as well is simply to optionally enabling building the Linux kernel with clang instead of GCC while preserving the full set of kernel features.

I know the existence of livepatch in the Linux kernel though unfamiliar with it. The kernel doesn't use -mno-pic-data-is-text-relative right now for any port. Why does SystemZ need -mno-pic-data-is-text-relative for livepatch?

This option is not present in the kernel sources, but it is used by the kpatch tool when building patch modules:
https://github.com/dynup/kpatch/blob/master/kpatch-build/kpatch-build

Whether or not this is needed on any platform depends on the particulars of the ISA, e.g. what is the span reachable via PC-relative instructions.

[efriedma]

I'm very confused how you expect this to work... messing with the linkage of global variables is almost certainly going to explode. If you need emit special sequences to access global variables with internal linkage, I'd expect the backend to handle that (along the lines of how -frwpi works on ARM).

[uweigand]

In D137044#3909116, @efriedma wrote:

I'm very confused how you expect this to work... messing with the linkage of global variables is almost certainly going to explode. If you need emit special sequences to access global variables with internal linkage, I'd expect the backend to handle that (along the lines of how -frwpi works on ARM).

I agree, this needs to be handled in the back end (it's done that way in the GCC implementation too). @jonpa can you have a look?

[jonpa]

Sorry for the confusion... My first idea to make an internal value external was based on the assumption that since no other module would ever actually be aware of it, it should be safe to play around with it and pretend that its linkage is external. Live patching in itself is weird enough to perhaps allow this kind of trick. But I guess then that it wasn't really a good idea at all... (as a newbie here I'm curious on what would break..?)

I tried another approach along your suggestions instead:

• Be compatible with GCC and accept -mpic-data-is-text-relative and -mno-pic-data-is-text-relative CL options.
• If Clang Driver gets -mno-pic-data-is-text-relative, it emits -no-pic-data-is-text-relative (just like -pic-level and -pic-is-pie) to the CmdArgs to CC1.
• If CC1 gets -no-pic-data-is-text-relative, a Module flag is emitted to indicate this (just like "PIC Level" and "PIE Level").
• If this module attribute is present, the SystemZ backend will access an internal global value via GOT.

[efriedma]

Makes sense to me. Maybe someone more familiar with the SystemZ side of things can confirm.

(as a newbie here I'm curious on what would break..?)

Off the top of my head:

• Other code could create internal globals
• Name conflict if multiple globals have the same name
• C++ name mangling might get confused.

llvm/lib/Target/SystemZ/SystemZISelLowering.cpp

3217 ↗

(On Diff #473507)

Move this check into isPC32DBLSymbol, maybe?

[MaskRay]

Making -mpic-data-is-text-relative unrecognized by non-SystemZ targets LG.

clang/test/Driver/pic.c

321 ↗

(On Diff #473507)

Use --target= for new tests. -target is legacy.

llvm/test/CodeGen/SystemZ/pic-data-is-text-rel.ll

1 ↗

(On Diff #473507)

You can use a trick like llvm/Instrumentation/InstrProfiling/comdat.ll to combine two files into one.

32 ↗

(On Diff #473507)

delete trailing blank line

[uweigand]

In D137044#3912908, @efriedma wrote:

Move this check into isPC32DBLSymbol, maybe?

Hmm. This is actually a very interesting point. isPC32DBLSymbol already implements exactly the behavior we want for this option, if -mcmodel=medium or -mcmodel=large is given. I hadn't thought about that.

So maybe instead of implementing a new module flag, the -mno-pic-data-is-text-relative option can be implemented solely in the front end or driver by mapping it to -mcmodel=medium?

(Unfortunately GCC on s390x does not implement -mcmodel, otherwise we might not have even needed the new option ...)

[jonpa]

Emit -mcmodel=medium instead from Driver.

[jonpa]

Check both versions of the option when emitting an error for unsupported option/target.

Off the top of my head:

Other code could create internal globals
Name conflict if multiple globals have the same name
C++ name mangling might get confused.

Ah, I see - that makes sense.

[uweigand]

LGTM, thanks!

[• pcwang-thead]

The test clang/test/Driver/pic.c failed when we compiled Clang/LLVM with -DCLANG_DEFAULT_PIE_ON_LINUX=False.

[jonpa]

In D137044#3942990, @pcwang-thead wrote:

The test clang/test/Driver/pic.c failed when we compiled Clang/LLVM with -DCLANG_DEFAULT_PIE_ON_LINUX=False.

Ah... I suppose that must be because -mno-pic-data-is-text-relative requires PIC (this is per GCC behavior). Does it help if you add -fpic to the RUN lines that were added by this patch? I was not aware of the cmake flag, sorry.

[Fznamznon]

In D137044#3943680, @jonpa wrote:

In D137044#3942990, @pcwang-thead wrote:

The test clang/test/Driver/pic.c failed when we compiled Clang/LLVM with -DCLANG_DEFAULT_PIE_ON_LINUX=False.

Ah... I suppose that must be because -mno-pic-data-is-text-relative requires PIC (this is per GCC behavior). Does it help if you add -fpic to the RUN lines that were added by this patch? I was not aware of the cmake flag, sorry.

For me it does. Can we make this change?

[jonpa]

In D137044#3943829, @Fznamznon wrote:

In D137044#3943680, @jonpa wrote:

In D137044#3942990, @pcwang-thead wrote:

The test clang/test/Driver/pic.c failed when we compiled Clang/LLVM with -DCLANG_DEFAULT_PIE_ON_LINUX=False.

Ah... I suppose that must be because -mno-pic-data-is-text-relative requires PIC (this is per GCC behavior). Does it help if you add -fpic to the RUN lines that were added by this patch? I was not aware of the cmake flag, sorry.

For me it does. Can we make this change?

Yes, that should be fine as the test is assuming pic by default. Thanks.

[Fznamznon]

In D137044#3944010, @jonpa wrote:

In D137044#3943829, @Fznamznon wrote:

In D137044#3943680, @jonpa wrote:

In D137044#3942990, @pcwang-thead wrote:

The test clang/test/Driver/pic.c failed when we compiled Clang/LLVM with -DCLANG_DEFAULT_PIE_ON_LINUX=False.

Ah... I suppose that must be because -mno-pic-data-is-text-relative requires PIC (this is per GCC behavior). Does it help if you add -fpic to the RUN lines that were added by this patch? I was not aware of the cmake flag, sorry.

For me it does. Can we make this change?

Yes, that should be fine as the test is assuming pic by default. Thanks.

I pushed the fix - https://github.com/llvm/llvm-project/commit/0ffcd243a42bff13966e455582d2db6c337628a2 . Thank you.

clang/include/clang/Basic/LangOptions.def

 VALUE_LANGOPT(AlignDouble            , 1, 0, "Controls if doubles should be aligned to 8 bytes (x86 only)")
 VALUE_LANGOPT(DoubleSize            , 32, 0, "width of double")
 VALUE_LANGOPT(LongDoubleSize        , 32, 0, "width of long double")
 LANGOPT(PPCIEEELongDouble            , 1, 0, "use IEEE 754 quadruple-precision for long double")
 LANGOPT(EnableAIXExtendedAltivecABI  , 1, 0, "__EXTABI__  predefined macro")
 LANGOPT(EnableAIXQuadwordAtomicsABI  , 1, 0, "Use 16-byte atomic lock free semantics")
 COMPATIBLE_VALUE_LANGOPT(PICLevel    , 2, 0, "__PIC__ level")
 COMPATIBLE_VALUE_LANGOPT(PIE         , 1, 0, "is pie")
+LANGOPT(PicDataIsTextRelative        , 1, 1, "Module-local data is text relative with PIC.")
 LANGOPT(ROPI                         , 1, 0, "Read-only position independence")
 LANGOPT(RWPI                         , 1, 0, "Read-write position independence")
 COMPATIBLE_LANGOPT(GNUInline         , 1, 0, "GNU inline semantics")
 COMPATIBLE_LANGOPT(NoInlineDefine    , 1, 0, "__NO_INLINE__ predefined macro")
 COMPATIBLE_LANGOPT(Deprecated        , 1, 0, "__DEPRECATED predefined macro")
 COMPATIBLE_LANGOPT(FastMath          , 1, 0, "fast FP math optimizations, and __FAST_MATH__ predefined macro")
 COMPATIBLE_LANGOPT(FiniteMathOnly    , 1, 0, "__FINITE_MATH_ONLY__ predefined macro")
 COMPATIBLE_LANGOPT(UnsafeFPMath      , 1, 0, "Unsafe Floating Point Math")

clang/include/clang/Driver/Options.td

def fms_hotpatch : Flag<["-"], "fms-hotpatch">, Group<f_Group>, Flags<[CC1Option, CoreOption]>,
   MarshallingInfoFlag<CodeGenOpts<"HotPatch">>;
 def fpcc_struct_return : Flag<["-"], "fpcc-struct-return">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Override the default ABI to return all structs on the stack">;
 def fpch_preprocess : Flag<["-"], "fpch-preprocess">, Group<f_Group>;
 def fpic : Flag<["-"], "fpic">, Group<f_Group>;
 def fno_pic : Flag<["-"], "fno-pic">, Group<f_Group>;
 def fpie : Flag<["-"], "fpie">, Group<f_Group>;
 def fno_pie : Flag<["-"], "fno-pie">, Group<f_Group>;
+def mno_pic_data_is_text_relative : Flag<["-"], "mno-pic-data-is-text-relative">, Group<m_Group>;
 def fdirect_access_external_data : Flag<["-"], "fdirect-access-external-data">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Don't use GOT indirection to reference external data symbols">;
 def fno_direct_access_external_data : Flag<["-"], "fno-direct-access-external-data">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Use GOT indirection to reference external data symbols">;
 defm plt : BoolFOption<"plt",
   CodeGenOpts<"NoPLT">, DefaultFalse,
   NegFlag<SetTrue, [CC1Option], "Use GOT indirection instead of PLT to make external function calls (x86 only)">,
   PosFlag<SetFalse>>;

 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">>;
+def no_pic_data_is_text_relative : Flag<["-"], "no-pic-data-is-text-relative">,
+  MarshallingInfoFlag<LangOpts<"PicDataIsTextRelative">>;
 
 } // let Flags = [CC1Option, FC1Option, NoDriverOption]
 
 let Flags = [CC1Option, NoDriverOption] in {
 
 def fblocks_runtime_optional : Flag<["-"], "fblocks-runtime-optional">,
   HelpText<"Weakly link in the blocks runtime">,
   MarshallingInfoFlag<LangOpts<"BlocksRuntimeOptional">>;

clang/lib/AST/ASTContext.cpp

     // Single source offloading languages like CUDA/HIP need to be able to
     // access static device variables from host code of the same compilation
     // unit. This is done by externalizing the static variable with a shared
     // name between the host and device compilation which is the same for the
     // same compilation unit whereas different among different compilation
     // units.
     if (Context.shouldExternalize(D))
       return GVA_StrongExternal;
-  }
+  } else if (Context.getLangOpts().PIE &&
+             !Context.getLangOpts().PicDataIsTextRelative &&
+             D->getKind() == Decl::Var && L == GVA_Internal)
+    // Treat GVs local to module as external with PIE if requested.
+    return GVA_StrongExternal;
+
   return L;
 }
 
 /// Adjust the GVALinkage for a declaration based on what an external AST source
 /// knows about whether there can be other definitions of this declaration.
 static GVALinkage
 adjustGVALinkageForExternalDefinitionKind(const ASTContext &Ctx, const Decl *D,
                                           GVALinkage L) {

clang/lib/Driver/ToolChains/Clang.cpp

if (RMName) {
     CmdArgs.push_back("-mrelocation-model");
     CmdArgs.push_back(RMName);
   }
   if (PICLevel > 0) {
     CmdArgs.push_back("-pic-level");
     CmdArgs.push_back(PICLevel == 1 ? "1" : "2");
     if (IsPIE)
       CmdArgs.push_back("-pic-is-pie");
+    if (Args.hasArg(options::OPT_mno_pic_data_is_text_relative))
+      CmdArgs.push_back("-no-pic-data-is-text-relative");
   }
 
   if (RelocationModel == llvm::Reloc::ROPI ||
       RelocationModel == llvm::Reloc::ROPI_RWPI)
     CmdArgs.push_back("-fropi");
   if (RelocationModel == llvm::Reloc::RWPI ||
       RelocationModel == llvm::Reloc::ROPI_RWPI)
     CmdArgs.push_back("-frwpi");

clang/lib/Frontend/CompilerInvocation.cpp

#undef LANG_OPTION_WITH_MARSHALLING
   // Validate options for HLSL
   if (Opts.HLSL) {
     bool SupportedTarget = T.getArch() == llvm::Triple::dxil &&
                            T.getOS() == llvm::Triple::ShaderModel;
     if (!SupportedTarget)
       Diags.Report(diag::err_drv_hlsl_unsupported_target) << T.str();
   }
 
+  Opts.PicDataIsTextRelative = !Args.hasArg(OPT_no_pic_data_is_text_relative);
+
   return Diags.getNumErrors() == NumErrorsBefore;
 }
 
 static bool isStrictlyPreprocessorAction(frontend::ActionKind Action) {
   switch (Action) {
   case frontend::ASTDeclList:
   case frontend::ASTDump:
   case frontend::ASTPrint: