Re: [PATCH 5/7] riscv: Fix feenvupdate with FE_DFL_ENV (BZ 31022)

[Bruno Haible <bruno@clisp.org>]

[-- Attachment #1: Type: text/plain, Size: 1062 bytes --]

This code gave me puzzles:

  long int env = (long int) envp - (long int) FE_DFL_ENV;
  if (env != 0)
    env = *envp;

* What is the intent of the code?
* Can signed integer overflow happen in the first line? (It cannot, due to
  the value of FE_DFL_ENV being -1 and envp being otherwise aligned mod 4.
  But one should not have to think that far.)
* It assumes that intptr_t is the same as 'long int'. Which is not
  universally true.

I would suggest to replace the function with this code:

libc_feupdateenv_riscv (const fenv_t *envp)
{
  long int env = (envp != FE_DFL_ENV ? *envp : 0);

  _FPU_SETCW (env | riscv_getflags ());
}

* It is clearer.
* It does not make assumptions regarding FE_DFL_ENV or alignment.
* It produces the exact same code; see attached foo.c and foo.s,
  generated by "riscv64-linux-gnu-gcc -O2 -S foo.c".

Probably the idiom that you copied was meant to allow the use of a
conditional load instruction. But I would avoid such micro-optimizations
and instead rely on the compiler to choose the best instructions for a
task.

Bruno

[-- Attachment #2: foo.c --]
[-- Type: text/x-csrc, Size: 632 bytes --]

typedef unsigned int fenv_t;
#define FE_DFL_ENV      ((__const fenv_t *) -1)
# define _FPU_SETCW(cw) __asm__ volatile ("fssr %z0" : : "rJ" (cw))

static __attribute__ ((__always_inline__)) int
riscv_getflags (void)
{
  int flags;
  asm volatile ("frflags %0" : "=r" (flags));
  return flags;
}

void
libc_feupdateenv_riscv_az (const fenv_t *envp)
{
  long int env = (long int) envp - (long int) FE_DFL_ENV;
  if (env != 0)
    env = *envp;

  _FPU_SETCW (env | riscv_getflags ());
}

void
libc_feupdateenv_riscv_bh (const fenv_t *envp)
{
  long int env = (envp != FE_DFL_ENV ? *envp : 0);

  _FPU_SETCW (env | riscv_getflags ());
}

[-- Attachment #3: foo.s --]
[-- Type: text/plain, Size: 839 bytes --]

	.file	"foo.c"
	.option pic
	.text
	.align	1
	.globl	libc_feupdateenv_riscv_az
	.type	libc_feupdateenv_riscv_az, @function
libc_feupdateenv_riscv_az:
	li	a5,-1
	li	a4,0
	beq	a0,a5,.L2
	lwu	a4,0(a0)
.L2:
#APP
# 9 "foo.c" 1
	frflags a5
# 0 "" 2
#NO_APP
	sext.w	a5,a5
	or	a5,a5,a4
#APP
# 20 "foo.c" 1
	fssr a5
# 0 "" 2
#NO_APP
	ret
	.size	libc_feupdateenv_riscv_az, .-libc_feupdateenv_riscv_az
	.align	1
	.globl	libc_feupdateenv_riscv_bh
	.type	libc_feupdateenv_riscv_bh, @function
libc_feupdateenv_riscv_bh:
	li	a5,-1
	li	a4,0
	beq	a0,a5,.L6
	lwu	a4,0(a0)
.L6:
#APP
# 9 "foo.c" 1
	frflags a5
# 0 "" 2
#NO_APP
	sext.w	a5,a5
	or	a5,a5,a4
#APP
# 28 "foo.c" 1
	fssr a5
# 0 "" 2
#NO_APP
	ret
	.size	libc_feupdateenv_riscv_bh, .-libc_feupdateenv_riscv_bh
	.ident	"GCC: (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0"
	.section	.note.GNU-stack,"",@progbits