Skip to content

[PAC][ELF] Place AUTH constants in .data.rel.ro for position dependent code #150418

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

[PAC][ELF] Place AUTH constants in .data.rel.ro for position dependent code #150418

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a91b0ee
[PAC][ELF] Place AUTH constants in .data.rel.ro for position dependen…
kovdan01 Jul 24, 2025
bf6186d
Merge branch 'main' into pauth-data-rel-ro
kovdan01 Sep 5, 2025
d85f8cf
Address review comments
kovdan01 Sep 5, 2025
1868cbe
Merge branch 'main' into pauth-data-rel-ro
kovdan01 Oct 19, 2025
cafc9df
Apply Anatoly's suggestion
kovdan01 Oct 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions llvm/lib/Target/TargetLoweringObjectFile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "llvm/IR/DataLayout.h"
#include "llvm/IR/DerivedTypes.h"
#include "llvm/IR/Function.h"
#include "llvm/IR/GlobalValue.h"
#include "llvm/IR/GlobalVariable.h"
#include "llvm/IR/Mangler.h"
#include "llvm/IR/Module.h"
Expand Down Expand Up @@ -226,6 +227,20 @@ void TargetLoweringObjectFile::emitPseudoProbeDescMetadata(
}
}

static bool containsConstantPtrAuth(const Constant *C) {
if (isa<ConstantPtrAuth>(C))
return true;

if (isa<BlockAddress>(C) || isa<GlobalValue>(C))
return false;
Comment on lines +234 to +235
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC this would make containsConstantPtrAuth return false for global variable like this

@global_var = global ptr ptrauth (ptr @f, i32 0)

which is still "constant" (as a constant pointer to global_var). This doesn't seem to hurt in case of this PR, but possibly a bit unexpected w.r.t. function name vs. its behavior.

Copy link
Contributor

@atrosinenko atrosinenko Oct 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, I would expect containsConstantPtrAuth to return false for @ptr_to_global_var = constant ptr @global_var no matter how @global_value is defined - I guess recursively inspecting GlobalValues would make containsConstantPtrAuth incorrectly return true in such cases. Thus, the original "issue" should be considered as a matter of comments or naming, if at all.


for (const Value *Op : C->operands())
if (containsConstantPtrAuth(cast<Constant>(Op)))
return true;

return false;
}

/// getKindForGlobal - This is a top-level target-independent classifier for
/// a global object. Given a global variable and information from the TM, this
/// function classifies the global in a target independent manner. This function
Expand Down Expand Up @@ -333,9 +348,10 @@ SectionKind TargetLoweringObjectFile::getKindForGlobal(const GlobalObject *GO,
// mergable section, because the linker doesn't take relocations into
// consideration when it tries to merge entries in the section.
Reloc::Model ReloModel = TM.getRelocationModel();
if (ReloModel == Reloc::Static || ReloModel == Reloc::ROPI ||
ReloModel == Reloc::RWPI || ReloModel == Reloc::ROPI_RWPI ||
!C->needsDynamicRelocation())
if ((ReloModel == Reloc::Static || ReloModel == Reloc::ROPI ||
ReloModel == Reloc::RWPI || ReloModel == Reloc::ROPI_RWPI ||
!C->needsDynamicRelocation()) &&
!containsConstantPtrAuth(C))
return SectionKind::getReadOnly();
Comment on lines 336 to 358
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Maybe it would be more readable to organize the checks along the lines

// ...
  } else {
    // First of all, the dynamic linker always needs to fix PtrAuth relocations up.
    if (containsPtrAuth(C))
      return SectionKind::getReadOnlyWithRel();

    // In static, ROPI and RWPI relocation models, the linker will resolve
    // ...
    if (ReloModel == ...)
      return SectionKind::getReadOnly();

    // Otherwise, the dynamic linker needs to fix it up, put it in the
    // writable data.rel section.
    return SectionKind::getReadOnlyWithRel();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've always found it a bit confusing that Constant::needsDynamicRelocation really means "needs dynamic relocation in non-static/R*PI relocation models". Would it make sense to move the check for relocation model as well as the ptrauth constant detection (without the reloc model check of course) into needsDynamicRelocation? Then this could end up being:

if (!C->needsDynamicRelocation(TM.getRelocationModel()))
  return SectionKind::getReadOnly();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Maybe it would be more readable to organize the checks along the lines

@atrosinenko Applied your suggestion in cafc9df, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to move the check for relocation model as well as the ptrauth constant detection (without the reloc model check of course) into needsDynamicRelocation?

@pcc While I like the idea of simplifying things and making naming less confusing, I'm not sure if such a change would make logic for all the users of needsDynamicRelocation simpler. Also, it looks like that some callers do not have easy access to TargetMachine object, so they can't easily call getRelocationModel() and pass that to new needsDynamicRelocation updated accordingly to your suggestion.

We might think of such an improvement, but I guess this is out of scope of this PR and it's better not to combine actual bug fixes and refactoring.

Can we postpone applying your suggestion? And could you please tell if there is any other feedback on this PR or this can be merged? Thanks!


// Otherwise, the dynamic linker needs to fix it up, put it in the
Expand Down
51 changes: 51 additions & 0 deletions llvm/test/CodeGen/AArch64/ptrauth-global-no-pic.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
; RUN: llc -mtriple aarch64-elf --relocation-model=static -mattr=+pauth < %s | FileCheck %s
; RUN: llc -mtriple aarch64-elf --relocation-model=dynamic-no-pic -mattr=+pauth < %s | FileCheck %s

;; A constant value, use .rodata
; CHECK: .section .rodata,"a",@progbits
; CHECK: .globl Const
; CHECK: Const:
; CHECK: .xword 37

;; An AUTH reloc is needed, use .data.rel.ro
; CHECK: .section .data.rel.ro,"aw",@progbits
; CHECK: .globl PtrAuthExtern
; CHECK: PtrAuthExtern:
; CHECK: .xword ConstExtern@AUTH(da,0)

; CHECK: .globl PtrAuth
; CHECK: PtrAuth:
; CHECK: .xword Const@AUTH(da,0)

; CHECK: .globl PtrAuthExternNested1
; CHECK: PtrAuthExternNested1:
; CHECK: .xword ConstExtern@AUTH(da,0)

;; The address could be filled statically, use .rodata
; CHECK: .section .rodata,"a",@progbits
; CHECK: .globl PtrAuthExternNested2
; CHECK: PtrAuthExternNested2:
; CHECK: .xword PtrAuthExtern

;; An AUTH reloc is needed, use .data.rel.ro
; CHECK: .section .data.rel.ro,"aw",@progbits
; CHECK: .globl PtrAuthNested1
; CHECK: PtrAuthNested1:
; CHECK: .xword Const@AUTH(da,0)

;; The address could be filled statically, use .rodata
; CHECK: .section .rodata,"a",@progbits
; CHECK: .globl PtrAuthNested2
; CHECK: PtrAuthNested2:
; CHECK: .xword PtrAuth

@ConstExtern = external global i64
@Const = constant i64 37

@PtrAuthExtern = constant ptr ptrauth (ptr @ConstExtern, i32 2)
@PtrAuth = constant ptr ptrauth (ptr @Const, i32 2)

@PtrAuthExternNested1 = constant { ptr } { ptr ptrauth (ptr @ConstExtern, i32 2) }
@PtrAuthExternNested2 = constant { ptr } { ptr @PtrAuthExtern }
@PtrAuthNested1 = constant { ptr } { ptr ptrauth (ptr @Const, i32 2) }
@PtrAuthNested2 = constant { ptr } { ptr @PtrAuth }
Loading