This should be reviewed by kib, andy (for ARM), jrtc27 (for riscv64), jhibbits (for ppc), and kargl (for msun). Please contact kargl by e-mail, he does not have a phabricator account.

lib/msun/arm/fenv.c
324	We don't support softfloat ARM anymore, so this can be omitted.

This should be reviewed by kib, andy (for ARM), jrtc27 (for riscv64), jhibbits (for ppc), and kargl (for msun).

Updating reviewers. Thanks!

Please contact kargl by e-mail, he does not have a phabricator account.

Sure, I will do.

Is it reasonable to provide the inline versions of these methods for complicated cases, like x86? Could it be always a function call?

lib/msun/amd64/fenv.c
45	Please use designated initializers.
lib/msun/i387/fenv.c
45	Can we use designated initializers please?
lib/msun/man/fegetmode.3
32	Why 'attempts'? There is no error defined for the implementation. I would say directly 'function stores the current control modes'. Same for fesetmode. I suspect that the language was directly copied from the standard. Also, it is the standard' requirement that fesetmode() can only be called on the result of the call to fegetmode(). Do we need to say that? We expose the structure, there is no hidden members.
lib/msun/x86/fenv.h
424	Why this cannot be `__fldcw(&__modep->__control)`?

kfv added inline comments.Tue, Sep 1, 12:15 AM

lib/msun/amd64/fenv.c
45	Sure.
lib/msun/i387/fenv.c
45	Sure, I'll take care of it sharpish.
lib/msun/man/fegetmode.3
32	Why 'attempts'? There is no error defined for the implementation. I would say directly 'function stores the current control modes'. Same for fesetmode. I suspect that the language was directly copied from the standard. Right, that was my oversight. Also, it is the standard' requirement that fesetmode() can only be called on the result of the call to fegetmode(). Do we need to say that? We expose the structure, there is no hidden members. I've tried to say it on lines 39-44, but please let me know we can should have it elsewhere as well or do it any better.
lib/msun/x86/fenv.h
424	Habit from the neighbouring inlines. I will update this and the i386 case shortly.

Address @kib's inline comments

Harbormaster completed remote builds in B76362: Diff 185528.Tue, Sep 1, 12:40 AM

kfv marked 4 inline comments as done.Tue, Sep 1, 12:42 AM

In D59288#1360218, @kib wrote:

Is it reasonable to provide the inline versions of these methods for complicated cases, like x86? Could it be always a function call?

This is still not answered.

lib/msun/Symbol.map
364	Order symbols alphabetically
lib/msun/man/fenv.3
199
lib/msun/x86/fenv.h
100	The __reserved half-word is added to explicitly account for padding, am I right? Why is it cleared by fesetmode()?

Is it reasonable to provide the inline versions of these methods for complicated cases, like x86? Could it be always a function call?

Sorry, I forgot to answer this one. Well, I think keeping the current split might be preferable; C inlines the CSR ops the same way fesetround() does, and C++ already takes the libm call. IMHO, fegetmode()/fesetmode() sit with that group, not with fegetenv()—please correct me if I'm wrong or missing something. That said, always calling libm would be fine too if you’d rather keep the complicated x86 sequence out of the header. I'm OK either way.

lib/msun/x86/fenv.h
100	Right, and `fegetmode()` zeroes it to ensure the snapshot is deterministic; `fesetmode()` never touches it. I did that thinking if `__reserved` is left alone while filling the other two, that could retain whatever the caller’s object already contained, and hence two saves of the same hardware state could differ. Please let me know if that is unnecessary.

Add new symbols in alphabetical order and apply @kib's fenv.3 suggestion

Harbormaster completed remote builds in B76382: Diff 185568.Tue, Sep 1, 3:29 PM

kfv marked 2 inline comments as done.Tue, Sep 1, 3:30 PM

In D59288#1360671, @kfv wrote:

Is it reasonable to provide the inline versions of these methods for complicated cases, like x86? Could it be always a function call?

Sorry, I forgot to answer this one. Well, I think keeping the current split might be preferable; C inlines the CSR ops the same way fesetround() does, and C++ already takes the libm call. IMHO, fegetmode()/fesetmode() sit with that group, not with fegetenv()—please correct me if I'm wrong or missing something. That said, always calling libm would be fine too if you’d rather keep the complicated x86 sequence out of the header. I'm OK either way.

For x86 I would even more care about expansion then complexity. Some time Intel might come with additional control bits for some XSTATE extension that is naturally fit into the fe*() area. But ok.

This revision is now accepted and ready to land.Tue, Sep 1, 8:07 PM

Approved for commit following @kib's review. I did not review the code, so only Approved by from me please.

In D59288#1360867, @fuz wrote:

Approved for commit following @kib's review. I did not review the code, so only Approved by from me please.

Sure. kargl also approved the patch by the way. I will land it in a little while.

lib/msun/arm/fenv.c
324	Right, but keeping them matches the existing split today. IMHO, ripping them out could be a separate process. I will take care of it and open another DR shortly.