From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.0 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.2 Received: from mother.openwall.net (mother.openwall.net [195.42.179.200]) by inbox.vuxu.org (OpenSMTPD) with SMTP id e0532af8 for ; Fri, 24 Jan 2020 01:11:36 +0000 (UTC) Received: (qmail 7404 invoked by uid 550); 24 Jan 2020 01:11:34 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 7377 invoked from network); 24 Jan 2020 01:11:34 -0000 Date: Thu, 23 Jan 2020 20:11:22 -0500 From: Rich Felker To: musl@lists.openwall.com Message-ID: <20200124011122.GP30412@brightrain.aerifal.cx> References: <20200116161427.GO30412@brightrain.aerifal.cx> <20200116193343.GP30412@brightrain.aerifal.cx> <20200117164143.GB2020@voyager> <20200118011535.GD23985@port70.net> <20200118053759.GX30412@brightrain.aerifal.cx> <20200118094015.GE23985@port70.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: Rich Felker Subject: Re: [musl] Considering x86-64 fenv.s to C On Fri, Jan 24, 2020 at 11:42:02AM +1100, Damian McGuckin wrote: > > In an attempt to address this issue, the following can be considered > an attempt at a 'discussion paper'. Thanks! I'm first commenting inline with the code since that's a more natural way for me to review, but I might come back with some comments on the text a bit later. Here it goes: > #ifndef FE_ALL_EXCEPT > #define FE_VALID_EXCEPT (FE_INEXACT|FE_DIV_BY_ZERO|FE_UNDERFLOW|FE_OVERFLOW) > #define FE_ALL_EXCEPT (FE_INVALID|FE_VALID_EXCEPT) > #endif I don't understand what this is for. FE_ALL_EXCEPT is defined in the public fenv.h and FE_VALID_EXCEPT is not used. > /* > * The usual approach to validating an exception mask is to throw out > * anything which is illegal. Some systems will NOT do this, choosing > * instead to return a non-zero result on encountering an illegal mask > */ The latter is not desirable behavior and not something you need to be trying to reproduce. It's a bug or at least an unspecified case we don't care about in existing code. > #ifndef FE_VALIDATE_EXCEPT > #define FE_VALIDATE_EXCEPT(e) e &= FE_ALL_EXCEPT > #endif Write things like this directly in code; don't hide them behind macros. Doing that just makes the code hard to read because you can't tell what it's doing without searching for the macro definition. (Also the macro is not protected by parens as written but that's easily fixable.) > /* > * There is usually no need to mess the generic valid exceptions bits > * given to 'feclearexcept' and 'feraiseexcept' so define a 'nothing' > * macro for such a scenario - is this the MUSL accepted style???? > */ > #ifndef FE_QUALIFY_CLEAR_EXCEPT > #define FE_QUALIFY_CLEAR_EXCEPT(excepts) (0) > #endif > #ifndef FE_QUALIFY_RAISE_EXCEPT > #define FE_QUALIFY_RAISE_EXCEPT(excepts) (0) > #endif Can you clarify how these would be defined for powerpc? Could the logic just be embedded in get_sr/set_sr if needed? > /* > * Compute the rounding mask with some rigid logic > */ > #ifndef FE_TO_NEAREST > #define FE_TO_NEAREST 0 > #define ROUND_MASK ((unsigned int) 0) > #else > #ifndef FE_TOWARDS_ZERO > #define ROUND_MASK ((unsigned int) 0) > #else > #ifdef FE_DOWNWARD > #ifdef FE_UPWARD > #define ROUND_MASK ((unsigned int) (FE_DOWNWARD|FE_UPWARD|FE_TOWARDZERO)) > #else > #define ROUND_MASK UNSUPPORTED rounding > #endif > #else > #ifdef FE_UPWARD > #define ROUND_MASK UNSUPPORTED rounding > #else > #define ROUND_MASK ((unsigned int) (FETOWARDZERO)) > #endif > #endif > #endif > #endif I don't see why you have "UNSUPPORTED rounding" cases here. Any combination should be valid. Since these are not bitmasks, probably the arch should just have to define the rounding mode mask. For example if the values were 0b000, 0b001, 0b100, and 0b101, but the 0 in the middle was significant, it should probably be treated as part of the mask. If you really want to generate the mask, something like #ifdef FE_TONEAREST #define FE_TONEAREST_MASK FE_TONEAREST #else #define FE_TONEAREST_MASK 0 #endif ... #define ROUND_MASK (FE_TONEAREST_MASK | FE_DOWNWARD_MASK | ...) should be simpler and comprehensive. > int > feclearexcept(excepts) > { > FE_VALIDATE_EXCEPT(excepts); > FE_QUALIFY_CLEAR_EXCEPT(excepts); > setsr(getsr() & ~((unsigned int) excepts)); > return(0); > } No function-like parens around return value, no gratuitous casts. > static inline int > __raisearithmetically(int excepts) > { > /* > * assume single OP is faster than double OP > */ > const float one = (float) 1; > const float zero = (float) 0; > const float tiny = (float) 0x1.0p-126; > const float huge = (float) 0x1.0p+126; > volatile float x; > > /* > * if it is just a simple exception, arithmetic expressions are optimal > */ > switch(excepts) > { > case FE_INVALID: > x = zero, x /= x; > break; > case FE_DIVBYZERO: > x = zero, x = one / x; > break; > case FE_INEXACT: > x = tiny, x += one; > break; > case (FE_OVERFLOW | FE_INEXACT): > x = huge, x *= x; > break; > case (FE_UNDERFLOW | FE_INEXACT): > x = tiny, x *= x; > break; > default: /* if more than one exception exists, a sledgehammer is viable */ > setsr(getsr() | ((unsigned int) excepts)); > break; > } > return(0); > } This is probably ok (aside from style things mentioned above), but simply recording the flags in software without writing them to the status register at all may be preferable. In that case we would not even need a set_sr primitive, only get_sr and clear_sr. Doing this would also simplify the ppc special-casing for FE_INVALID I think.. > int > feraiseexcept(int excepts) > { > FE_VALIDATE_EXCEPT(excepts); > FE_QUALIFY_RAISE_EXCEPT(excepts); > return __raisearithmetically(excepts); > } Then this could just be __pthread_self()->fpsr |= excepts; > int > fetestexcept(int excepts) > { > FE_VALIDATE_EXCEPT(excepts); > > return (int) (getsr() & ((unsigned int) excepts)); > } > int > fegetround(void) > { > return (int) (getcr() & ROUND_MASK); > } > > int > fesetround(int rounding_mode) > { > if ((rounding_mode & ~ROUND_MASK) == 0) > { > unsigned int mode = ((unsigned int) rounding_mode); > > return (setcr((getcr() & ~ROUND_MASK) | (mode & ROUND_MASK)), 0); > } > return(-1); > } I would put the error test at the beginning rather than putting the whole body inside {}: if (rounding_mode & ~ROUND_MASK) return -1; > int > fegetenv(fenv_t *envp) > { > return ((gete(envp)), 0); > } Is there a reason for using comma operator here? > int > fesetenv(fenv_t *envp) > { > fenv_t envpd = FE_DFL_ENV_DATA, *e = envp == FE_DFL_ENV ? &envpd : envp; > > return ((sete(envp)), 0); > } It might be preferable to use a static const object for default env; I'm not sure. Not a big deal either way though.