[XRay] Add generic patchable-function-entry NOP sled implementation
Needs ReviewPublic
Actions

Authored by duck-37 on Feb 10 2023, 7:33 PM.

Details

Reviewers

Summary

This patch generalizes the logic for patchable-function-entry=n NOP sleds, meaning that this attribute now has consistent behavior across all architectures that support XRay. The differences in codegen was noticed while porting a separate test-case over to ARM32.

Closes https://github.com/llvm/llvm-project/issues/60672

Diff Detail

Repository: rG LLVM Github Monorepo

Unit TestsFailed
View All

	Time	Test
	60,170 ms	x64 debian > Clang.Driver::arm-cortex-cpus-1.c Script: -- : 'RUN: at line 7'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -target arm -mcpu=generic -### -c /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/arm-cortex-cpus-1.c 2>&1 \| /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck -check-prefix=CHECK-GENERIC /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/arm-cortex-cpus-1.c
	60,370 ms	x64 debian > Clang.Driver::arm-cortex-cpus-2.c Script: -- : 'RUN: at line 8'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -target armv8a-linux-eabi -mcpu=cortex-a53+fp16 -### -c /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/arm-cortex-cpus-2.c 2>&1 \| /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck --check-prefix CHECK-CORTEX-A53-FP16 /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/arm-cortex-cpus-2.c
	60,130 ms	x64 debian > Clang.Driver::crash-report.cpp Script: -- : 'RUN: at line 1'; rm -rf /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Driver/Output/crash-report.cpp.tmp
	60,250 ms	x64 debian > Clang.Driver::emit-reproducer.c Script: -- : 'RUN: at line 1'; rm -rf /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Driver/Output/emit-reproducer.c.tmp && mkdir /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Driver/Output/emit-reproducer.c.tmp
	60,430 ms	x64 debian > Clang.Driver::fsanitize.c Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang --target=x86_64-linux-gnu -fsanitize=undefined -fsanitize-trap=undefined /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/fsanitize.c -### 2>&1 \| /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/fsanitize.c --check-prefix=CHECK-UNDEFINED-TRAP
		View Full Test Results (7 Failed)

Event Timeline

duck-37 created this revision.Feb 10 2023, 7:33 PM

Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2023, 7:33 PM

Herald added subscribers: luke, frasercrmck, luismarques and 23 others. · View Herald Transcript

duck-37 requested review of this revision.Feb 10 2023, 7:33 PM

Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2023, 7:33 PM

Herald added subscribers: llvm-commits, pcwang-thead. · View Herald Transcript

duck-37 retitled this revision from [XRay] Make patchable-function-entry NOP sled implementation target-independent This patch generalizes the logic for patchable-function-entry NOP sleds, which extends support for them to all architectures that support XRay. A consequence of this... to [XRay] Make patchable-function-entry NOP sled implementation target-independent.Feb 10 2023, 7:34 PM

duck-37 edited the summary of this revision. (Show Details)

The increased instruction count for x86 is a slight performance regression. Can it be fixed?

Fixes https://github.com/llvm/llvm-project/issues/60672

It's not a bug so I'd use Close. Note: without runtime support, the compiler codegen change isn't really useful

In D143802#4119979, @MaskRay wrote:

Fixes https://github.com/llvm/llvm-project/issues/60672

It's not a bug so I'd use Close. Note: without runtime support, the compiler codegen change isn't really useful

Sure, I'll adjust that for the next revision. The main idea here was consistency across architectures; I came across this while moving a test-case over to ARM32 and noticing a difference in the behavior.

duck-37 planned changes to this revision.Feb 11 2023, 9:44 AM

Changed some wording in the review, used a virtual function instead of hardcoded emitNops call so that X86 (and potentially other targets) can use their own implementations if necessary.

Herald added a subscriber: kristof.beyls. · View Herald TranscriptFeb 11 2023, 9:50 AM

Harbormaster completed remote builds in B213223: Diff 496694.Feb 11 2023, 10:37 AM