Re: [PATCH] loongarch: Only select HAVE_OBJTOOL and allow ORC unwinder if the inline assembler supports R_LARCH_{32,64}_PCREL

From: Jinyang He
Date: Wed Jun 05 2024 - 11:48:10 EST


This is a multi-part message in MIME format. On 2024-06-05 23:13, Xi Ruoyao wrote:

On Wed, 2024-06-05 at 21:18 +0800, Xi Ruoyao wrote:
On Wed, 2024-06-05 at 18:57 +0800, Xi Ruoyao wrote:
On Tue, 2024-06-04 at 23:25 -0700, Nathan Chancellor wrote:
On Wed, Jun 05, 2024 at 01:54:24PM +0800, Xi Ruoyao wrote:
On Tue, 2024-06-04 at 22:43 -0700, Nathan Chancellor wrote:
For what it's worth, I have noticed some warnings with clang that I
don't see with GCC but I only filed an issue on our GitHub and never
followed up on the mailing list, so sorry about that.

https://github.com/ClangBuiltLinux/linux/issues/2024

Might be tangential to this patch though but I felt it was worth
mentioning.
The warnings in GCC build is definitely the issue handled by this patch.
But the warnings in Clang build should be a different issue.  Can you
attach the kernel/events/core.o file from the Clang build for analysis?
I guess we need to disable more optimization...
Sure thing. Let me know if there are any issues with the attachment.
Thanks!  I've simplified it and now even...

.global test
.type test,@function
test:

addi.d $sp,$sp,-448
st.d $ra,$sp,440
st.d $fp,$sp,432
addi.d $fp,$sp,448

# do something

addi.d $sp,$fp,-448
ld.d $fp,$sp,432
ld.d $ra,$sp,440
addi.d $sp,$sp,448
ret

.size test,.-test

is enough to trigger a objtool warning:

/home/xry111/t1.o: warning: objtool: test+0x20: return with modified stack frame

And to me this warning is bogus?
Minimal C reproducer:

struct x { _Alignas(64) char buf[128]; };

void f(struct x *p);
void g()
{
struct x x = { .buf = "1145141919810" };
f(&x);
}

Then objtool is unhappy to the object file produced with "clang -c -O2"
from this translation unit:

/home/xry111/t2.o: warning: objtool: g+0x50: return with modified stack frame

It seems CFI_BP has a very specific semantic in objtool and Clang does
not operates $fp in the expected way.  I'm not sure about my conclusion
though.  Maybe Peter can explain it better.
Another example: some simple rust code:

extern { fn f(x: &i64) -> i64; }

#[no_mangle]
fn g() -> i64 {
let x = 114514;
unsafe {f(&x)}
}

It's just lucky GCC doesn't use $fp as the frame pointer unless there's
some stupid code (VLA etc) thus the issue was not detected.


I think this because we did not add arch special handle in update_cfi_state().
In our 419 repo this func has been renamed arch_update_insn_state (, now it
should be arch_update_cfi_state) and give some actions to deal with the
frame pointer case. If we support it we may deal with some case but for clang...

.global test
.type test,@function
test:

addi.d  $sp,$sp,-448
st.d    $ra,$sp,440
st.d    $fp,$sp,432
addi.d  $fp,$sp,448

# do something  <- not change $sp

# addi.d        $sp,$fp,-448  <- not restore sp from cfa(fp)
ld.d    $fp,$sp,432
ld.d    $ra,$sp,440
addi.d  $sp,$sp,448
ret

.size test,.-test

This will cause warning, too. (Gcc should be ok, I don't test.)

source.c: (clang 19, based 0c1c0d53931, -g)
void *foo() { return __builtin_frame_address(0); }

asm.s:
        .cfi_sections .debug_frame
        .cfi_startproc
# %bb.0:
        addi.d  $sp, $sp, -16
        .cfi_def_cfa_offset 16
        st.d    $ra, $sp, 8                     # 8-byte Folded Spill
        st.d    $fp, $sp, 0                     # 8-byte Folded Spill
        .cfi_offset 1, -8
        .cfi_offset 22, -16
        addi.d  $fp, $sp, 16
        .cfi_def_cfa 22, 0                    <- define cfa is $r22
.Ltmp0:
        .loc    0 1 22 prologue_end             # hello.c:1:22
        move    $a0, $fp
        ld.d    $fp, $sp, 0                     # 8-byte Folded Reload  <- restore regs from non-cfa ?
        ld.d    $ra, $sp, 8                     # 8-byte Folded Reload  <- restore regs from non-cfa ?
        .loc    0 1 15 epilogue_begin is_stmt 0 # hello.c:1:15
        addi.d  $sp, $sp, 16                                            <- restore prev-sp from non-cfa ?
        ret

Maybe Clang have anything wrong?


Attach: tmp fix by support arch_update_insn_state.


Thanks,

Jinyang