On Thu, Jun 5, 2025 at 4:12 PM Richard Sandiford
<richard.sandif...@arm.com> wrote:
>
> "H.J. Lu" <hjl.to...@gmail.com> writes:
> > On Wed, Jun 4, 2025 at 4:13 PM Richard Sandiford
> > <richard.sandif...@arm.com> wrote:
> >>
> >> Richard Biener <richard.guent...@gmail.com> writes:
> >> > On Wed, Jun 4, 2025 at 7:28 AM H.J. Lu <hjl.to...@gmail.com> wrote:
> >> >>
> >> >> On s390x, for input:
> >> >>
> >> >> (call_insn/u 7 6 11 2 (parallel [
> >> >> (set (reg:SI 2 %r2)
> >> >> (call (subreg:QI (symbol_ref:SI ("__tls_get_offset")
> >> >> [flags 0x1]) 3)
> >> >> (const_int 0 [0])))
> >> >> (clobber (reg:SI 14 %r14))
> >> >> (use (unspec:SI [
> >> >> (const_int 0 [0])
> >> >> ] UNSPEC_TLSLDM))
> >> >> ]) "/tmp/foo.c":12:26 2602 {*brasl_tls}
> >> >> (expr_list:REG_EH_REGION (const_int -2147483648
> >> >> [0xffffffff80000000])
> >> >> (nil))
> >> >> (expr_list (use (reg:SI 2 %r2))
> >> >> (expr_list (use (reg:SI 12 %r12))
> >> >> (nil))))
> >> >>
> >> >> after r16-1041-g2da641d0170090, get_call_rtx_from returns:
> >> >>
> >> >> (call (subreg:QI (symbol_ref:SI ("__tls_get_offset") [flags 0x1]) 3)
> >> >> (const_int 0 [0]))
> >> >
> >> > That's a strange call!
> >>
> >> Yeah. Are we sure it's really correct? Taken literally, it says that
> >> we're interpreting the symbol __tls_get_offset as a sequence of
> >> instructions.
> >>
> >> Also, the docs say:
> >>
> >> @item (call @var{function} @var{nargs})
> >> Represents a function call. @var{function} is a @code{mem} expression
> >> whose address is the address of the function to be called.
> >>
> >> So I don't object to the patch, but it seems like it might be papering
> >> over a target bug. cc:ing s390 maintainers.
> >>
> >> Richard
> >
> > After r16-1041-g2da641d0170090, get_call_rtx_from returns
> >
> > (call (unspec:SI [
> > (mem:SI (reg/f:SI 98) [0 MEM[(void (*<T2f6>) (struct
> > test_st))209715
> > 2B] S4 A32])
> > ] UNSPEC_NONSECURE_MEM)
> > (const_int 0 [0]))
> >
> > on
> >
> > (call_insn 14 13 0 (parallel [
> > (call (unspec:SI [
> > (mem:SI (reg/f:SI 98) [0 MEM[(void (*<T2f6>)
> > (struct tes
> > t_st))2097152B] S4 A32])
> > ] UNSPEC_NONSECURE_MEM)
> > (const_int 0 [0]))
> > (use (const_int 0 [0]))
> > (clobber (reg:SI 14 lr))
> > ])
> > "/export/gnu/import/git/sources/gcc/gcc/testsuite/gcc.target/arm/cmse
> > /mainline/8_1m/../../bitfield-4.x":38:3 -1
> > (nil)
> > (nil))
> >
> > Update to check MEM_P before using MEM_EXPR in emit_call_1 and
> > rest_of_insert_endbr_and_patchable_area.
> >
> > PR other/120493
> > * calls.cc (emit_call_1): Use MEM_EXPR only if MEM_P is true.
> > * config/i386/i386-features.cc
> > (rest_of_insert_endbr_and_patchable_area): Likewise.
>
> Not sure, but: did you mean to attach a patch?
Oops. Here is the attached patch.
> But the call above also doesn't follow the documentation, in that
> the operand of the call is an unspec rather than a mem. So if we
> want to bless this kind of call, we should at least update the
> documentation to say what is now allowed.
>
> FWIW, if the intent is to use the unspec to attach on-the-side
> information to the call, I think it'd better to do that in parallel
> with the call, rather than as an operand to it. I suppose that's
> what the:
>
> (use (const_int 0 [0]))
>
> is already doing for other information (and is also what AArch64 uses
> to record the PCS of the target function).
>
> Thanks,
> Richard
--
H.J.
From 9df7b6cd4dc0536126e78497cd7531b89d280771 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.to...@gmail.com>
Date: Thu, 5 Jun 2025 05:02:15 +0800
Subject: [PATCH] More use MEM_EXPR only if MEM_P is true
After r16-1041-g2da641d0170090, get_call_rtx_from returns
(call (unspec:SI [
(mem:SI (reg/f:SI 98) [0 MEM[(void (*<T2f6>) (struct test_st))2097152B] S4 A32])
] UNSPEC_NONSECURE_MEM)
(const_int 0 [0]))
on
(call_insn 14 13 0 (parallel [
(call (unspec:SI [
(mem:SI (reg/f:SI 98) [0 MEM[(void (*<T2f6>) (struct test_st))2097152B] S4 A32])
] UNSPEC_NONSECURE_MEM)
(const_int 0 [0]))
(use (const_int 0 [0]))
(clobber (reg:SI 14 lr))
]) "/export/gnu/import/git/sources/gcc/gcc/testsuite/gcc.target/arm/cmse/mainline/8_1m/../../bitfield-4.x":38:3 -1
(nil)
(nil))
Update to check MEM_P before using MEM_EXPR in emit_call_1 and
rest_of_insert_endbr_and_patchable_area.
PR other/120493
* calls.cc (emit_call_1): Use MEM_EXPR only if MEM_P is true.
* config/i386/i386-features.cc
(rest_of_insert_endbr_and_patchable_area): Likewise.
Signed-off-by: H.J. Lu <hjl.to...@gmail.com>
---
gcc/calls.cc | 1 +
gcc/config/i386/i386-features.cc | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/gcc/calls.cc b/gcc/calls.cc
index e16190c12a6..7c6b8ad6bbc 100644
--- a/gcc/calls.cc
+++ b/gcc/calls.cc
@@ -481,6 +481,7 @@ emit_call_1 (rtx funexp, tree fntree ATTRIBUTE_UNUSED, tree fndecl ATTRIBUTE_UNU
above. Set its MEM_EXPR. */
call = get_call_rtx_from (call_insn);
if (call
+ && MEM_P (XEXP (call, 0))
&& MEM_EXPR (XEXP (call, 0)) == NULL_TREE
&& MEM_EXPR (funmem) != NULL_TREE)
set_mem_expr (XEXP (call, 0), MEM_EXPR (funmem));
diff --git a/gcc/config/i386/i386-features.cc b/gcc/config/i386/i386-features.cc
index b1682c2fad4..d12a4bde8e3 100644
--- a/gcc/config/i386/i386-features.cc
+++ b/gcc/config/i386/i386-features.cc
@@ -2955,7 +2955,7 @@ rest_of_insert_endbr_and_patchable_area (bool need_endbr,
may return via indirect branch. */
if (GET_CODE (XEXP (fnaddr, 0)) == SYMBOL_REF)
fndecl = SYMBOL_REF_DECL (XEXP (fnaddr, 0));
- if (fndecl == NULL_TREE)
+ if (fndecl == NULL_TREE && MEM_P (fnaddr))
fndecl = MEM_EXPR (fnaddr);
if (fndecl
&& TREE_CODE (TREE_TYPE (fndecl)) != FUNCTION_TYPE
--
2.49.0
