Page MenuHomeFreeBSD

[POWERPC] Handles float point exception
Needs ReviewPublic

Authored by renato.riolino_eldorado.org.br on Tue, Feb 11, 6:33 PM.

Details

Reviewers
bdragon
jhibbits
Summary

Enables FPE on ppc by default. Note: user application still needs to set, with feenableexcept() , which exception will trigger a SIGFPE.

Test Plan

#include <fenv.h>

int main(void)
{

feenableexcept(FE_ALL_EXCEPT);
double x = 0.0;
double y = 1.0 / x;
return 0;

}

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 29313
Build 27216: arc lint + arc unit

Event Timeline

jhibbits added inline comments.Tue, Feb 11, 10:25 PM
sys/powerpc/include/psl.h
49

This belongs in trap.hh, as EXC_PGM_FPE, or similar name, since it's part of SRR1, not MSR.

Actually, I think this really should belong in spr.h, as SRR1_PGM_FPE, and the other EXC_PGM* #defines moved there, too. But that's not how it's done now, so just put it in trap.h.

Looks good.
I just have some style related comments and 2 questions about the changes.

sys/powerpc/include/psl.h
91

Do we want to enable FP exceptions by default?

sys/powerpc/powerpc/trap.c
400–425

This brace should be in the line above.
See style(9).

406

Variable declarations should be all in the beginning of a block, separated of other statements by a new line, and function call initializers should not be used (e.g. mfmsr()).
See style(9).

426

I may be missing something, but why is enable_fpu() needed here?

renato.riolino_eldorado.org.br marked 3 inline comments as done.Wed, Feb 12, 1:02 PM
renato.riolino_eldorado.org.br added inline comments.
sys/powerpc/include/psl.h
91

It's still not "enabled". By default none of the exception enabled registers are enable. User must call feenableexcept() if he wants to receive a FP exception.

sys/powerpc/powerpc/trap.c
426

enable_fpu() loads the floating point registers from the PCB. Without it the application can't capture the SIGFPE.

Fixes pointed by jhibbits and luporl and a test of endianess when checking the type of fp exception.

luporl added inline comments.Wed, Feb 12, 1:43 PM
sys/powerpc/powerpc/trap.c
402–413

I think this whole union could be replaced by an uint64_t.
The only part where you use it as a double is in the inline assembly, which should work fine with uint64_t too.
Then you wouldn't need to worry about the endianness too.
(BTW, currently all kernel PPC code assume BE, but starting to take LE into account is nice).

Using a uint64_t instead of a union as suggested by luporl

jhibbits added inline comments.Sat, Feb 15, 2:47 PM
sys/powerpc/powerpc/trap.c
413

This block won't work as you think. FPE_FLTDIV, et al, are integers, not bitfields. Only one reason is allowed.

jhibbits added inline comments.Sat, Feb 15, 2:48 PM
sys/powerpc/powerpc/trap.c
424

Is there a paired save_fpu() anywhere? Otherwise we're overwriting the FPRs with whatever was last saved, which could be nothing.