This is an archive of the discontinued LLVM Phabricator instance.

Differential D158491

Sanitizer/MIPS: fix build fail on pre-R6
Needs ReviewPublic

Authored by wzssyqa on Aug 22 2023, 12:35 AM.

Details

Reviewers
MaskRay
brad
jrtc27
FlyGoat
Summary

On MIPS pre-R6, instruction b can only work within 64KiB,
which is not enough now.

We need the help of GOT.
For __mips64, we can get GOT by:

lui $t8, %hi(%neg(%gp_rel(SANITIZER_STRINGIFY(TRAMPOLINE(func)))))
daddu $t8, $t8, $t9
daddiu $t8, $t8, %hi(%neg(%gp_rel(SANITIZER_STRINGIFY(TRAMPOLINE(func)))))

And then get the address of __interceptor_func, and jump to it

ld $t9, %got_disp(__interceptor_" SANITIZER_STRINGIFY(func) ")($t8)
jr $t9

MIPS/O32 has .cpload, which can help to generate 3 instructions to get GOT.

MIPSr6 has instruction bc, which can jump long enough.

Diff Detail

Event Timeline

wzssyqa created this revision.Aug 22 2023, 12:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2023, 12:35 AM
Herald added subscribers: Enna1, atanasyan, arichardson, sdardis. · View Herald Transcript
wzssyqa requested review of this revision.Aug 22 2023, 12:35 AM
Herald added a subscriber: wangpc. · View Herald TranscriptAug 22 2023, 12:35 AM
Harbormaster completed remote builds in B254009: Diff 552237.Aug 22 2023, 12:52 AM
wzssyqa mentioned this in D158268: [Sanitizer] Add MIPS vfork interception.Aug 22 2023, 12:55 AM
MaskRay added a comment.Aug 22 2023, 11:08 AM

This large pile of mips code makes me nervous. What I really want is to eliminate code related to some older mips ports.
For example, just claim that defined(__mips__) && __mips_isa_rev < 6 is not supported for sanitizers.

jrtc27 requested changes to this revision.Aug 22 2023, 11:11 AM

Why not define a SANITIZER_TAIL_CALL(sym) that expands to the assembly string to tail call sym? Give it the default implementation of the current ASM_TAIL_CALL sym and then add MIPS overrides as needed. That should drastically reduce the amount of boilerplate; this copy-pasta is not good.

This revision now requires changes to proceed.Aug 22 2023, 11:11 AM
wzssyqa updated this revision to Diff 552595.Aug 22 2023, 11:13 PM
wzssyqa added a comment.Aug 22 2023, 11:24 PM
In D158491#4607577, @MaskRay wrote:

This large pile of mips code makes me nervous. What I really want is to eliminate code related to some older mips ports.
For example, just claim that defined(__mips__) && __mips_isa_rev < 6 is not supported for sanitizers.

Pre-R6 is what currently widely used mostly.
I wish it can be supported.

wzssyqa updated this revision to Diff 552599.Aug 22 2023, 11:28 PM
Harbormaster completed remote builds in B254258: Diff 552599.Aug 22 2023, 11:43 PM
wzssyqa updated this revision to Diff 552649.Aug 23 2023, 3:52 AM
wzssyqa edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B254292: Diff 552649.Aug 23 2023, 4:11 AM
wzssyqa updated this revision to Diff 553382.Aug 24 2023, 11:47 PM
MaskRay added a reviewer: FlyGoat.Aug 25 2023, 12:04 AM
Harbormaster completed remote builds in B254811: Diff 553382.Aug 25 2023, 12:16 AM
jrtc27 added a comment.Aug 25 2023, 12:31 AM

Please upload patches with full context per https://llvm.org/docs/DeveloperPolicy.html#making-and-submitting-a-patch

compiler-rt/lib/sanitizer_common/sanitizer_asm.h
48

One space not two

49

Should mips be removed?

59

One space for these

60

No spaces before \n

62

Why $t8 rather than another temporary or $gp?

wzssyqa added inline comments.Aug 25 2023, 12:41 AM
compiler-rt/lib/sanitizer_common/sanitizer_asm.h
49

Should mips be removed?

No. Since ASM_INTERCEPTOR_TRAMPOLINE uses it.
For full asm functions, like vfork, the distance of these 2 functions is quite near.
b is enough for it.
See https://reviews.llvm.org/D158268

62

Why $t8 rather than another temporary or $gp?

In fact, I don't know. I tested with $gp, while something wrong (segfault) always happen.
I have no idea why...

wzssyqa updated this revision to Diff 553398.Aug 25 2023, 12:45 AM
wzssyqa updated this revision to Diff 553401.Aug 25 2023, 12:48 AM
wzssyqa marked 4 inline comments as done.
wzssyqa marked an inline comment as done.
Harbormaster completed remote builds in B254825: Diff 553401.Aug 25 2023, 1:51 AM
jrtc27 added a comment.Aug 25 2023, 10:29 AM

Ah, I went and reminded myself of the MIPS ABI. N32/N64 has GP as call-preserved. O32 has GP as call-preserved for non-PIC, call-clobbered for PIC.

Also %got_disp forces eager binding and is a function pointer. If this can be adapted to use %call16 that would be better. I assume there's something similar for O32 but am not familiar enough with it.

wzssyqa added a comment.Oct 10 2023, 8:10 PM

ping

sylvestre.ledru added a subscriber: sylvestre.ledru.Mon, Nov 27, 2:45 AM

@MaskRay do you think it could be merged now ? thanks
i have been told that it is necessary for Debian

wzssyqa added a comment.Tue, Nov 28, 1:19 AM

ping. This problem affects gcc now.

glaubitz added a subscriber: glaubitz.Sat, Dec 2, 5:20 PM

Revision Contents

PathSize
compiler-rt/
lib/
interception/
5 lines
sanitizer_common/
21 lines

Diff 553382

compiler-rt/lib/interception/interception.h

Context not available.
".type " SANITIZER_STRINGIFY(TRAMPOLINE(func)) ", %function\n" \ ".type " SANITIZER_STRINGIFY(TRAMPOLINE(func)) ", %function\n" \
SANITIZER_STRINGIFY(TRAMPOLINE(func)) ":\n" \ SANITIZER_STRINGIFY(TRAMPOLINE(func)) ":\n" \
SANITIZER_STRINGIFY(CFI_STARTPROC) "\n" \ SANITIZER_STRINGIFY(CFI_STARTPROC) "\n" \
SANITIZER_STRINGIFY(ASM_TAIL_CALL) " __interceptor_" \ C_ASM_TAIL_CALL(SANITIZER_STRINGIFY(TRAMPOLINE(func)), \
SANITIZER_STRINGIFY(ASM_PREEMPTIBLE_SYM(func)) "\n" \ "__interceptor_" \
SANITIZER_STRINGIFY(ASM_PREEMPTIBLE_SYM(func))) "\n" \
SANITIZER_STRINGIFY(CFI_ENDPROC) "\n" \ SANITIZER_STRINGIFY(CFI_ENDPROC) "\n" \
".size " SANITIZER_STRINGIFY(TRAMPOLINE(func)) ", " \ ".size " SANITIZER_STRINGIFY(TRAMPOLINE(func)) ", " \
".-" SANITIZER_STRINGIFY(TRAMPOLINE(func)) "\n" \ ".-" SANITIZER_STRINGIFY(TRAMPOLINE(func)) "\n" \
Context not available.

compiler-rt/lib/sanitizer_common/sanitizer_asm.h

Context not available.
#if defined(__x86_64__) || defined(__i386__) || defined(__sparc__) #if defined(__x86_64__) || defined(__i386__) || defined(__sparc__)
# define ASM_TAIL_CALL jmp # define ASM_TAIL_CALL jmp
#elif defined(__mips__) && __mips_isa_rev >= 6
# define ASM_TAIL_CALL bc
jrtc27Unsubmitted
Done
ReplyInline Actions

One space not two

jrtc27: One space not two
#elif defined(__arm__) || defined(__aarch64__) || defined(__mips__) || \ #elif defined(__arm__) || defined(__aarch64__) || defined(__mips__) || \
jrtc27Unsubmitted
Done
ReplyInline Actions

Should mips be removed?

jrtc27: Should mips be removed?
wzssyqaAuthorUnsubmitted
Done
ReplyInline Actions

Should mips be removed?

No. Since ASM_INTERCEPTOR_TRAMPOLINE uses it.
For full asm functions, like vfork, the distance of these 2 functions is quite near.
b is enough for it.
See https://reviews.llvm.org/D158268

wzssyqa: > Should mips be removed? No. Since `ASM_INTERCEPTOR_TRAMPOLINE` uses it. For full asm…
defined(__powerpc__) || defined(__loongarch_lp64) defined(__powerpc__) || defined(__loongarch_lp64)
# define ASM_TAIL_CALL b # define ASM_TAIL_CALL b
Context not available.
# define ASM_TAIL_CALL tail # define ASM_TAIL_CALL tail
#endif #endif
#if defined(__mips64) && __mips_isa_rev < 6
# define C_ASM_TAIL_CALL(tfunc, ifunc) \
jrtc27Unsubmitted
Done
ReplyInline Actions

One space for these

jrtc27: One space for these
"lui $t8, %hi(%neg(%gp_rel(" tfunc "))) \n" \
jrtc27Unsubmitted
Done
ReplyInline Actions

No spaces before \n

jrtc27: No spaces before \n
"daddu $t8, $t8, $t9 \n" \
"daddu $t8, $t8, %lo(%neg(%gp_rel(" tfunc "))) \n" \
jrtc27Unsubmitted
Done
ReplyInline Actions

Why $t8 rather than another temporary or $gp?

jrtc27: Why $t8 rather than another temporary or $gp?
wzssyqaAuthorUnsubmitted
Done
ReplyInline Actions

Why $t8 rather than another temporary or $gp?

In fact, I don't know. I tested with $gp, while something wrong (segfault) always happen.
I have no idea why...

wzssyqa: > Why $t8 rather than another temporary or $gp? In fact, I don't know. I tested with $gp…
"ld $t9, %got_disp(" ifunc ")($t8) \n" \
"jr $t9 \n"
#elif defined(__mips__) && __mips_isa_rev < 6
# define C_ASM_TAIL_CALL(tfunc, ifunc) \
".set noreorder \n" \
".cpload $t9 \n" \
".set reorder \n" \
"lw $t9, %got(" ifunc ")($gp) \n" \
"jr $t9 \n"
#elif defined(ASM_TAIL_CALL)
# define C_ASM_TAIL_CALL(tfunc, ifunc) \
SANITIZER_STRINGIFY(ASM_TAIL_CALL) " " ifunc
#endif
#if defined(__ELF__) && defined(__x86_64__) || defined(__i386__) || \ #if defined(__ELF__) && defined(__x86_64__) || defined(__i386__) || \
defined(__riscv) defined(__riscv)
# define ASM_PREEMPTIBLE_SYM(sym) sym@plt # define ASM_PREEMPTIBLE_SYM(sym) sym@plt
Context not available.