Page MenuHomePhabricator

[AArch64] Add function attribute "patchable-function-entry" to add NOPs at function entry
ClosedPublic

Authored by MaskRay on Jan 4 2020, 3:02 PM.

Details

Summary

The Linux kernel uses -fpatchable-function-entry to implement DYNAMIC_FTRACE_WITH_REGS
for arm64 and parisc. GCC 8 implemented
-fpatchable-function-entry, which can be seen as a generalized form of
-mnop-mcount. The N,M form (function entry points before the Mth NOP) is
currently only used by parisc.

This patch adds N,0 support to AArch64 codegen. N is represented as the
function attribute "patchable-function-entry". We will use a different
function attribute for M, if we decide to implement it.

The patch reuses the existing patchable-function pass, and
TargetOpcode::PATCHABLE_FUNCTION_ENTER which is currently used by XRay.

When the integrated assembler is used, __patchable_function_entries will
be created for each text section with the SHF_LINK_ORDER flag to prevent
--gc-sections (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93197) and
COMDAT (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93195) issues.

"patchable-function-entry"'s interaction with Branch Target
Identification is still unclear (see
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92424 for GCC discussions).

Diff Detail

Event Timeline

MaskRay created this revision.Jan 4 2020, 3:02 PM

Unit tests: pass. 61249 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

How does this interact with the AArch64 "branch-target-enforcement" attribute? For the generated, unpatched code to be correct the function needs to start with a BTI or PAC instruction if it could be indirectly called. Presumably the code doing the patching would also need to be aware of this.

The GCC docs claim that a __patchable_function_entries section will be emitted containing the addresses of every run of NOPs inserted, do you plan on adding that too?

llvm/lib/IR/Verifier.cpp
1862

I think it would be better to use separate attributes for the size and start offset, to avoid needing to do this parsing.

MaskRay updated this revision to Diff 236485.Jan 6 2020, 5:17 PM

Address comments.

The section __patchable_function_entries needs more thoughts so it is not
implemented. I'll probably use SHF_LINK_ORDER when -ffunction-sections, and add
SHF_WRITE (to prevent text relocations; apparently GCC does not consider these
issues).

MaskRay edited the summary of this revision. (Show Details)Jan 6 2020, 5:18 PM
MaskRay marked an inline comment as done.

Unit tests: pass. 61272 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

MaskRay updated this revision to Diff 236528.Jan 6 2020, 11:53 PM

Add __patchable_function_entries section

GCC's implementation has 2 problems: (1) it does not set the SHF_WRITE flag https://gcc.gnu.org/ml/gcc/2020-01/msg00048.html (2) sh_addralign is not pointersize

Unit tests: pass. 61275 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

The N,M form (function entry points before the Mth NOP) is
currently only used by parisc.

As a heads-up, I have rough plans to use the N,M form for the arm64 Linux kernel in the near future. We may need this so that we can allocate trampolines or literal pools next too patch sites for live-patching and/or large test kernels.

This patch adds N,0 support to AArch64 codegen. N is represented as the
function attribute "patchable-function-entry". We will use a different
function attribute for M, if we decide to implement it.

Other than the interaction with BTI, is implementing the full N,M form difficult today?

I'll poke the GCC folk w.r.t. the BTI issues.

Address comments.

The section __patchable_function_entries needs more thoughts so it is not
implemented. I'll probably use SHF_LINK_ORDER when -ffunction-sections, and add
SHF_WRITE (to prevent text relocations; apparently GCC does not consider these
issues).

It looks like we'll need to co-design this with GCC, binutils and the kernel to get something that works across the toolchains, it looks like this has already started https://www.mail-archive.com/gcc@gcc.gnu.org/msg90391.html. I think SHF_LINK_ORDER would work in LLD, but I'm not sure it would have the desired behaviour under garbage collection in ld.bfd. In any case I think that this could be implemented in a follow up patch.

I think N,M support could be potentially added in a later patch as well, given Mark's comment. I think that if the N, M support isn't initially implemented it will be worth giving a "not-supported error message".

MaskRay added a subscriber: reames.Jan 7 2020, 4:33 PM

@reames Is D19046 the LLVM function attribute "patchable-function" and the target opcode PATCHABLE_OP used by any projects? Sanjoy said you may know.

Address comments.

The section __patchable_function_entries needs more thoughts so it is not
implemented. I'll probably use SHF_LINK_ORDER when -ffunction-sections, and add
SHF_WRITE (to prevent text relocations; apparently GCC does not consider these
issues).

It looks like we'll need to co-design this with GCC, binutils and the kernel to get something that works across the toolchains, it looks like this has already started https://www.mail-archive.com/gcc@gcc.gnu.org/msg90391.html. I think SHF_LINK_ORDER would work in LLD, but I'm not sure it would have the desired behaviour under garbage collection in ld.bfd. In any case I think that this could be implemented in a follow up patch.

I only got two issues with GCC's __patchable_function_entries now: (1) it does not have the SHF_WRITE flag (2) sh_addralign=1 (instead of pointer-size). This patch is otherwise identical to my observable effects of GCC.

I think N,M support could be potentially added in a later patch as well, given Mark's comment. I think that if the N, M support isn't initially implemented it will be worth giving a "not-supported error message".

Yes. I think clangDriver is a better a place to error "unsupported". See D72222. I also mark -fxray-instrument incompatible there.

MaskRay updated this revision to Diff 236772.EditedJan 8 2020, 12:23 AM

I think GCC's __patchable_function_entries has severe design flaws. https://gcc.gnu.org/ml/gcc/2020-01/msg00067.html

I have tried a different design in this patch, temporarily naming it __patchable_function_entry.

Unit tests: pass. 61307 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

MaskRay updated this revision to Diff 236774.Jan 8 2020, 1:00 AM

Learn from .stack_sizes

Unit tests: pass. 61307 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

I think GCC's __patchable_function_entries has severe design flaws. https://gcc.gnu.org/ml/gcc/2020-01/msg00067.html

I have tried a different design in this patch, temporarily naming it __patchable_function_entry.

FWIW I think that this is definitely an improvement over the original. This should permit the patchable_function_entries to be read-only even when compiled position independent and should deal with dangling entries caused by GC and comdat entries. We may need an option to emit in the old format for compatibility, but I guess we'll need to see how the GCC community reacts.

llvm/test/CodeGen/AArch64/patchable-function-entry.ll
21

It might be worth a run of llc with -ffunction-sections where we'd expect to see f1, unique, 0.

chill resigned from this revision.Jan 8 2020, 2:07 AM
MaskRay updated this revision to Diff 236956.Jan 8 2020, 7:57 PM

Add -function-sections test

Unit tests: pass. 61321 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

MaskRay updated this revision to Diff 237248.EditedJan 9 2020, 11:34 PM

@peter.smith @ostannard PTAL.

Support -no-integrated-as

Unfortunately as of binutils 2.33, GNU as does not support the "o" flag (SHF_LINK_ORDER).

Switch from PC-relative relocation to symbolic relocation

R_MIPS_PC64 does not exist, so I don't dislike -fpatchable-function-entry= that much now.
If GCC/Linux devs want to add -fpatchable-function-entry=2,0,pcrel , we can follow, and it should be straightforward to implement that.

I believe Linux's -fpatchable-function-entry=2 Makefile is not clang friendly.
It does not add this option automatically.

But anyhow, applying D72222 and all its depedencies does not break defconfig or allyesconfig.
So this patch series should be safe to merge.

MaskRay marked an inline comment as done.Jan 9 2020, 11:36 PM

Unit tests: pass. 61743 tests passed, 0 failed and 780 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

peter.smith accepted this revision.Jan 10 2020, 3:54 AM

LGTM thanks for the update.

This revision is now accepted and ready to land.Jan 10 2020, 3:54 AM
MaskRay updated this revision to Diff 237364.Jan 10 2020, 9:47 AM
MaskRay edited the summary of this revision. (Show Details)

Update description

This revision was automatically updated to reflect the committed changes.

Unit tests: pass. 61758 tests passed, 0 failed and 780 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

@reames Is D19046 the LLVM function attribute "patchable-function" and the target opcode PATCHABLE_OP used by any projects? Sanjoy said you may know.

Sorry, for the slow response.

Yes, the existing attribute is in place. The difference between the two based on a quick read is that patchable-function expects a destructive patch (i.e. replacing an existing instruction) whereas the new code expects a non-destructive (i.e. replace nops) patch. There could be some more communing in the implementation and naming, but I have no problem with supporting both.

And yes, "patchable-function" is in use. We use it for target redirection when replacing a piece of compiled code with another.