From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x629.google.com (mail-pl1-x629.google.com [IPv6:2607:f8b0:4864:20::629]) by sourceware.org (Postfix) with ESMTPS id 352A13858D32 for ; Fri, 24 Nov 2023 17:53:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 352A13858D32 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 352A13858D32 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::629 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1700848438; cv=none; b=nMEYgENumOJ5d4E0Dfyjeacczp3wuTLKFcZUu6ozywFel6AT2KIpvAgLswl9wZ3SlkmIaxy7GzeaqvPOhlu9sVaIfyJnanu8rbhnpbZtvGhXOOz+u//2i7thPXoDemgoH8MAR4DvHqvVJxL82MbgTHWq2TQFh9TQpCizAmteqs4= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1700848438; c=relaxed/simple; bh=MTIFKZAG+mSppGra9WMreN6iPpgVOPZVbtr7EbOliGo=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=ehGzLp693OB5ot6mWL/xovHC5MwcOs7tqGZN2u9yiyrcb4fgWA9SRPR3cqTqYAM3wg/TtYed7HtqH9MdQ4xW+PWPc8+Z+JVMl4ThxfUaxdZSkFSlABSzEiGneKhpLnMiSpADlVzQx771oudaFoNYcgTHUw9KqRtBgjX0BZLBg8c= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pl1-x629.google.com with SMTP id d9443c01a7336-1cfb2176150so229225ad.3 for ; Fri, 24 Nov 2023 09:53:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1700848435; x=1701453235; darn=sourceware.org; h=content-transfer-encoding:in-reply-to:organization:from:references :to:content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=dnGHZ/W47UURf/LPvdoKwFgIBEXjF2m+8GZ5ezT04c0=; b=pgRE5cnn16pr61LHQ8FwyL2BTgmpgQE2qfMdBg67kwGAN3Cffdx6MDHOv410n7Zh7I 1p6cjVU6cXhLnghBVhg6GqPIl5KofdLkrVCeB7PnujDA8E03I9hO6Pp/Hr7FDoCQiwSs xN/ewmKDN7439z2cb8GFmAmfLDvOr3ZeN1ESDYHF0DfQQFo7aXs4/g+Ntdbd0VQaEM8h OfvSnhnffHEMif0R8XWFfnGpztNyW1oDhVJBeiN1wDNW4WVDAqDkf+iYWTKTIQAkNlig 4uvT4xwxDJCrFp01Ohu1mlmmeIW8bThr4K0w+47zqsmG4lXvGs92eJGAhIKyr+Gi5ZwW RSJQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700848435; x=1701453235; h=content-transfer-encoding:in-reply-to:organization:from:references :to:content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=dnGHZ/W47UURf/LPvdoKwFgIBEXjF2m+8GZ5ezT04c0=; b=Zzxbmf60N6ZMewBz8cFY7/ED/EnNDc301AbUOFeYmAsv4qML69XlICp9wU2AOyEUsP X8CAmqRNvzkNk6UdWIUVJ9T/384iftJITFInEPY+dHfP4G0/RVa+QR2hbb19Ju542IzY ktcaK2eRFgpDEYMQmqzPoLEtyOoWS+8a4p4OvaK2UFpPE/jvBGLeXhrA5E4dJPv+2s7x 50403qBIJ73fx8+mSzEG2mu+y0tLgumCbHjcub/kiMIU+PlssHr79RXQunteNOzpyiXm TYbAlDy+xVJbwd1w6u5PJZ3qpRL0F3o+riEhPJBhAWSFrWmJA53rdQSFZLLrKxh5EFNB g3yw== X-Gm-Message-State: AOJu0YwdeT2Gvvd6dKlSI4edKVEKDty4NhX8aNLMxO6kdjAkL10/6fxy v3Lre7cTw674COEEbfeGFXqUJJq7nPrb2+eucS+NUw== X-Google-Smtp-Source: AGHT+IG96LZNYsSkgsCHuHKl/y0GQE1/VLJfxBaMkZ/R47S4aezD4zAhsBMHg3oIIzI4UVE4eN2EPg== X-Received: by 2002:a17:902:c38c:b0:1ce:89a7:440b with SMTP id g12-20020a170902c38c00b001ce89a7440bmr3520685plg.2.1700848434991; Fri, 24 Nov 2023 09:53:54 -0800 (PST) Received: from ?IPV6:2804:1b3:a7c2:94e:6d59:cb73:1a0b:30ad? ([2804:1b3:a7c2:94e:6d59:cb73:1a0b:30ad]) by smtp.gmail.com with ESMTPSA id jl14-20020a170903134e00b001cf6453b237sm3455267plb.236.2023.11.24.09.53.53 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 24 Nov 2023 09:53:54 -0800 (PST) Message-ID: Date: Fri, 24 Nov 2023 14:53:51 -0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 1/7] powerpc: Do not raise exception traps for fesetexcept/fesetexceptflag (BZ 30988) Content-Language: en-US To: Carlos O'Donell , libc-alpha@sourceware.org, Bruno Haible References: <20231106132713.953501-1-adhemerval.zanella@linaro.org> <20231106132713.953501-2-adhemerval.zanella@linaro.org> <6130f4c9-dab2-6f8e-5bc5-902b5a48e2dc@redhat.com> <5e031d35-5d3e-49a7-b354-809bb4a1dc8f@linaro.org> <6e4ff3c5-8504-79a4-8865-0239b0cd7185@redhat.com> <8aaf2565-5310-44aa-a331-6d12b26d2274@linaro.org> <3052e518-dac2-4b23-a070-787db5e13bf2@linaro.org> <6e52fa2f-9198-4a0a-9c77-80a1eccbd347@linaro.org> <0f892926-e336-f08d-c96e-0ad38d4ce75a@redhat.com> From: Adhemerval Zanella Netto Organization: Linaro In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 24/11/23 13:22, Carlos O'Donell wrote: > On 11/24/23 07:28, Adhemerval Zanella Netto wrote: >> It won't fail on powerpc (I actually tested using the gcc compile farm), because >> EXCEPTION_TESTS (float) won't be checked: >> >> volatile double a = 1.0; >> volatile double b = a + a; >> math_force_eval (b); // It will trigger the exception >> volatile long double al = 1.0L; >> volatile long double bl = al + al; >> math_force_eval (bl); >> >> if (ret == 0) // ret will -1 here (this very fix) > > OK. Agreed. > >> puts ("fesetexcept (FE_ALL_EXCEPT) succeeded"); >> else if (!EXCEPTION_SET_FORCES_TRAP) // EXCEPTION_SET_FORCES_TRAP is set to 1 > > OK. Agreed. > >> { >> puts ("fesetexcept (FE_ALL_EXCEPT) failed"); >> if (EXCEPTION_TESTS (float)) >> { >> puts ("failure of fesetexcept was unexpected"); >> result = 1; > > Where do we set EXCEPTION_TESTS (float) to zero for POWER? > > sysdeps/generic/math-tests-exceptions.h:#define EXCEPTION_TESTS_float 1 > sysdeps/generic/math-tests-exceptions.h:#define EXCEPTION_TESTS_double 1 > sysdeps/generic/math-tests-exceptions.h:#define EXCEPTION_TESTS_long_double 1 > sysdeps/generic/math-tests-exceptions.h:#define EXCEPTION_TESTS_float128 1 We don't, powerpc does support exceptions. The issues is a powerpc limitation that Bruno has pointed out in BZ 30988: setting a floating-point exception flag triggers a trap, when traps are enabled for the particular exception and globally for the thread (via prctl (PR_SET_FPEXC, PR_FP_EXC_PRECISE)). It is because feenableexcept on powerpc enables the PR_FP_EXC_PRECISE mode. > > >> } >> else >> puts ("failure of fesetexcept OK"); >> } >> >>> >>> Let me sketch out what I was expecting for both test cases: >>> >>> diff --git a/math/test-fesetexcept-traps.c b/math/test-fesetexcept-traps.c >>> index 71b6e45b33..5ea295a5b8 100644 >>> --- a/math/test-fesetexcept-traps.c >>> +++ b/math/test-fesetexcept-traps.c >>> @@ -23,46 +23,97 @@ >>> static int >>> do_test (void) >>> { >>> - int result = 0; >>> + int errors = 0; >>> + int ret; >>> >>> fedisableexcept (FE_ALL_EXCEPT); >>> - int ret = feenableexcept (FE_ALL_EXCEPT); >>> + ret = feenableexcept (FE_ALL_EXCEPT); >>> if (!EXCEPTION_ENABLE_SUPPORTED (FE_ALL_EXCEPT) && (ret == -1)) >>> { >>> - puts ("feenableexcept (FE_ALL_EXCEPT) not supported, cannot test"); >>> + puts ("UNSUPPORTED: feenableexcept (FE_ALL_EXCEPT) not supported, cannot test"); >>> return 77; >>> } >>> else if (ret != 0) >>> { >>> - puts ("feenableexcept (FE_ALL_EXCEPT) failed"); >>> - result = 1; >>> - return result; >>> + puts ("FAIL: feenableexcept (FE_ALL_EXCEPT)"); >>> + errors++; >>> + return errors; >>> } >>> >>> - if (EXCEPTION_SET_FORCES_TRAP) >>> + if (!EXCEPTION_SET_FORCES_TRAP) >>> { >>> - puts ("setting exceptions traps, cannot test on this architecture"); >>> - return 77; >>> + /* Verify fesetexcept does not cause exception traps. */ >>> + ret = fesetexcept (FE_ALL_EXCEPT); >>> + if (ret == 0) >>> + puts ("PASS: fesetexcept (FE_ALL_EXCEPT)"); >>> + else >>> + { >>> + /* Some architectures are expected to fail. */ >>> + if (EXCEPTION_TESTS (float)) >>> + puts ("PASS: fesetexcept (FE_ALL_EXCEPT) " >>> + "failed as expected because testing is disabled"); >>> + else >>> + { >>> + puts ("FAIL: fesetexcept (FE_ALL_EXCEPT)"); >>> + errors++; >>> + } >>> + } >>> + ret = feclearexcept (FE_ALL_EXCEPT); >>> + if (ret == 0) >>> + puts ("PASS: feclearexcept (FE_ALL_EXCEPT)"); >>> + else >>> + { >>> + /* Some architectures are expected to fail. */ >>> + if (EXCEPTION_TESTS (float)) >>> + { >>> + puts ("PASS: feclearexcept (FE_ALL_EXCEPT) " >>> + "failed as expected because testing is disabled"); >>> + } >>> + else >>> + { >>> + puts ("FAIL: feclearexcept (FE_ALL_EXCEPT) failed"); >>> + errors++; >>> + } >>> + } >>> } >>> - /* Verify fesetexcept does not cause exception traps. */ >>> - ret = fesetexcept (FE_ALL_EXCEPT); >>> - if (ret == 0) >>> - puts ("fesetexcept (FE_ALL_EXCEPT) succeeded"); >>> else >>> { >>> - puts ("fesetexcept (FE_ALL_EXCEPT) failed"); >>> - if (EXCEPTION_TESTS (float)) >>> + /* Verify fesetexcept fails because the hardware cannot set the >>> + exceptions without also raising them. */ >>> + ret = fesetexcept (FE_ALL_EXCEPT); >>> + if (ret == 0) >>> { >>> - puts ("failure of fesetexcept was unexpected"); >>> - result = 1; >>> + puts ("FAIL: fesetexcept (FE_ALL_EXCEPT) succeeded unexpectedly"); >>> + errors++; >>> } >> >> I think this is essentially what you think my proposed change is incomplete, >> I assume that EXCEPTION_SET_FORCES_TRAP is a hit since I think it might be >> possible that either kernel might paper over this limitation (by some instruction >> emulation to hide the exception signal) or a new chip revision might eventually >> fix it (as i686 did with SSE2). >> >> Maybe it would be better to assume that EXCEPTION_SET_FORCES_TRAP is a failure >> expectation and trigger a regression is function succeeds. > > Correct, if the function succeeds then it is a failure, it's likely someone broke > the conditional and now we have a function that is back to raising traps by > accident like it was before. It is a regression of bug 30988 if it succeeds. Agreed. > >> >>> else >>> - puts ("failure of fesetexcept OK"); >>> + { >>> + if (EXCEPTION_TESTS (float)) >>> + puts ("PASS: fesetexcept (FE_ALL_EXCEPT) " >>> + "failed as expected because testing is disabled"); >>> + else >>> + puts ("PASS: fesetexcept (FE_ALL_EXCEPT) failed as expected"); >>> + } >>> + ret = feclearexcept (FE_ALL_EXCEPT); >>> + if (ret == 0) >>> + puts ("PASS: feclearexcept (FE_ALL_EXCEPT)"); >>> + else >>> + { >>> + /* Some architectures are expected to fail. */ >>> + if (EXCEPTION_TESTS (float)) >>> + { >>> + puts ("PASS: feclearexcept (FE_ALL_EXCEPT) " >>> + "failed as expected because testing is disabled"); >>> + } >>> + else >>> + { >>> + puts ("FAIL: feclearexcept (FE_ALL_EXCEPT) failed"); >>> + errors++; >>> + } >>> + } >>> } >>> - feclearexcept (FE_ALL_EXCEPT); >>> >>> - return result; >>> + return errors; >>> } >>> >>> -#define TEST_FUNCTION do_test () >>> -#include "../test-skeleton.c" >>> +#include >>> --- >>> >>> My point is that by changing the implementation we need to test a whole >>> different set of conditions now and the test needs expanding, likewise >>> with test-fexcept-traps.c. >>> >>> We need two testing paths with different expectations? >> >> No really, the whole point of the test is to check: >> >> int exc_before = fegetexcept (); >> ret = fesetexcept (FE_ALL_EXCEPT); >> int exc_after = fegetexcept (); >> >> Will not change the exception mask (exc_before == exc_after) *and* not generate >> any trap (which you abort the process). Also, for i686 we need to trigger some >> math operations after the fesetexcept to check no exception will be triggered. >> >> Now, if ret is 0 everything works as expected. If ret is -1, it would depend >> whether the architecture has EXCEPTION_SET_FORCES_TRAP: >> >> * if is not set, it will depend whether the architectures allows setting >> the exception for the specific float type (EXCEPTION_TESTS (float), which >> is expanded to the constants defined by math-tests-exceptions.h). Some >> architectures does not support exceptions at all (riscv), or it depends >> of the ABI (arc, arm, loongarch, and ork1 in soft-fp mode). > > Agreed. > >> >> * if it is set (powerpc and i386/x87) it means that there is no extra >> checks requires, since the failure for these architectures *is* >> expected. > > Agreed. Though EXCEPTION_TESTS is still relevant here to avoid regression. > >> >> So assuming EXCEPTION_SET_FORCES_TRAP is a hard indication, I think this >> below would be suffice: >> >> if (ret == 0) >> puts ("fesetexcept (FE_ALL_EXCEPT) succeeded"); >> else if (!EXCEPTION_SET_FORCES_TRAP) >> { >> puts ("fesetexcept (FE_ALL_EXCEPT) failed"); >> if (EXCEPTION_TESTS (float)) >> { >> puts ("failure of fesetexcept was unexpected"); >> result = 1; >> } >> else >> puts ("failure of fesetexcept OK"); >> } >> else >> { >> if (ret == 0) >> puts ("unexpected fesetexcept success"); >> result = ret != -1; >> } >> > > Pasted below from downthread correction: > >> Oops, the above does not make sense: >> >> if (ret == 0) >> { >> if (EXCEPTION_SET_FORCES_TRAP) >> { >> puts ("unexpected fesetexcept success"); >> result = 1; > > Yes, this catches a POWER target regression for bug 30988. > > For the sake of completeness and the use of internal macro APIs > it is conceivable that EXCEPTION_TESTS could be used to check if the > test should even be checked (like my suggestion does).> > I consider it a simplification that you are applying target > knowledge from other files in the tree to skip that check > i.e. you know there is no EXCEPTION_SET_FORCES_TRAP true target that > is also EXCEPTION_TESTS true target. > > Is it correct to apply that simplification to this code? > > Or should the code handle both EXCEPTION_SET_FORCES_TRAP and > EXCEPTION_TESTS permutations in a generic fashion? I think the simplification applies here, because the issue is a powerpc/x87 architecture limitation here setting the floating-point register status will trigger a floating point exception (x87 would trigger in the next floating point operation, but it is essentially the same issue). So the fesetexcept/fesetexceptflag would either: 1. Raise a floating point exception, aborting the testcase (current code). 2. Fail where it should not. 3. Rail where it should (powerpc/x87). 4. Succeeds. So 1. and 2. are considered a regression, where 3. and 4. is the expected result. > >> } >> } >> else if (!EXCEPTION_SET_FORCES_TRAP) > > OK. This is all other architecture failure paths. > >> { >> puts ("fesetexcept (FE_ALL_EXCEPT) failed"); > > OK. > >> if (EXCEPTION_TESTS (float)) >> { >> puts ("failure of fesetexcept was unexpected"); >> result = 1; > > OK. This is the failure path for all targets that can do these operations. > >> } >> else >> puts ("failure of fesetexcept OK"); > > OK. Because it shouldn't be tested e.g. np-fpu targets. > >> } > > >