From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f49.google.com (mail-wm1-f49.google.com [209.85.128.49]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id DB9151E5706 for ; Mon, 20 Jan 2025 18:41:47 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.49 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737398509; cv=none; b=c13wfmLc1m1026IeOjirXq6glKod9mWsw8DXDE3fIbGrePLOZ593YmZsVBGBa+CyJx+pu+xAnK7H6JUnSguzhmMlr1Wsp0cIHOnllGfmV8HB1OJDyj63G7BgCQ3Klg0xL1uUvtMxWVllODL/LPD35j1xxE0Wvs2uN/8TvY1CcsU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737398509; c=relaxed/simple; bh=HYyPm7sZ2o/xwBFZGmhz0zkDrWIJfVB1qO9Ta4tm9yg=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=tyjoG/KNN5wTEONhefrlF2g5LDOp3SMnYon5UHu6x9GaPXIXCJFme0+kI2SclRH9ur58JebnNsqCeI/xBwxRv1Jo/9Vmhoep4PmF/5OWpNNEPh8vETdg7+eGbxNJVoQ+lnzuypz2T/YOWRKeJW1mUlMsfKIwtYFonia6KGHUazs= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=Z6bETsti; arc=none smtp.client-ip=209.85.128.49 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="Z6bETsti" Received: by mail-wm1-f49.google.com with SMTP id 5b1f17b1804b1-43618283dedso49199905e9.3 for ; Mon, 20 Jan 2025 10:41:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1737398506; x=1738003306; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=DIBwpgd4nZZVUOe5igOTj25p3iSQrZ0QRAo6bvYlPtM=; b=Z6bETstiHWQK/Yj3zCr5AQu0skcr6KPsifET9kduqe66HkTNPJkV0xZV8qrgCJi+pl +xEHeeNl3LqvfA4cfQrWr19Ru7V5gbxdpeJZlzz8xaCZT/ONMM8RoPENzqSLVm90x+Kr 33UST7srrRNDYkqpUNnOUOk160UEG6Io24ftbA+4va2NemH0H3UmrJnP+Bsvqh2EEQ3D dqNK1Na0Io8nt+JXTSatUO7fR5U4bq7yClyH3JtZGaf6QQt+0aWtuo8+BSAoe/rhlxWy rfoJDCtcnyIQx4iycogst/W3MowCXPHOZtaf0bZd5GjD7jXtgN2DJF3PAo7oXawzjsfk V8Hg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1737398506; x=1738003306; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=DIBwpgd4nZZVUOe5igOTj25p3iSQrZ0QRAo6bvYlPtM=; b=CdEEdy8a039X31IltvdIn7zuMNWnnum/nExT8ipoxcyqQQAyta5q2Cj2C5drrlpz7T K+ZJ+khzQM/RriERLY59E/yx93BXVaoe/FnCNnOMwML4Je5QOUnTZBrrT1W2PAFzOTyP AkVLUfDuoaAk3BFRL1qx491sdYPLsHKFttbyWt9XR4aLVo/Ek5/6abWSiKFXDQjpfs/A Ra4qaCEAps+EgF0KwGxBqXzhvNqgVYoTzcoxxoN5xe6hEtj/HRv//KT6f9LVdzw3c/nH oVoI6dr179rO2KtTdBScJC9R+0ra2EBEJ5D6Q5q3jRR7M9GUGZELW8WVLTI+5ldRsIHn 9GvA== X-Forwarded-Encrypted: i=1; AJvYcCX1aNLUgYBzwBjMUZVJfdSJm0vdfm+bP3pY2mASYKD7UDN2gLmQY8OrRbNVsEEUxkmnwCL7lSSilG2rXbs=@vger.kernel.org X-Gm-Message-State: AOJu0YyEKdkzC6muzrYLgxifrG+0DztlnObvOQ9wJQ8jHULkL2MsC5BA BdqviCZemP4IDpEbxJ4VVJaakfe7jDzhFK5/WaS6Jcg0PS5rSocW X-Gm-Gg: ASbGncsTZrRtn+6Urc7CkQBHYNWnBhy6D4cXxZZRwi7IhHVuw3xR1328H/QvOMzcJ+z OtEdxkRsKwNXt/MAmumC9s/b/acE92ggaO5QK2Y+qLjTk+S4dFkloFr8SgOqKn9ESag8vjH/oXy zdJBhXZxKvtA2DYMIMDNKDVC0IeQodr/2spQV+O2P6wuXSXsHi1V9y06Glbtnqsn6a5+ufuyYBu LPqkAohKH+kMbCfLwATvp4W3ybWdHoAGnY2NlGtYKIfRRLXcaAwAEExJJSkdKDhWljidCk7zJDR 6sGX3ZsKphVbdyhUsdk8smRIizww1rxj X-Google-Smtp-Source: AGHT+IEn3fWuuB2LneTt6yQ3PstmeBWVOQXwkDbyDPfhPGTU6q73G1l86IByTH3i/KUxwcvQA0z7UA== X-Received: by 2002:a05:600c:4f05:b0:434:fe62:28c1 with SMTP id 5b1f17b1804b1-438913ef83bmr125983915e9.18.1737398505721; Mon, 20 Jan 2025 10:41:45 -0800 (PST) Received: from pumpkin (82-69-66-36.dsl.in-addr.zen.co.uk. [82.69.66.36]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-43890462195sm148506615e9.30.2025.01.20.10.41.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 20 Jan 2025 10:41:45 -0800 (PST) Date: Mon, 20 Jan 2025 18:41:43 +0000 From: David Laight To: Guenter Roeck Cc: Jani Nikula , Linus Torvalds , David Laight , Arnd Bergmann , "linux-kernel@vger.kernel.org" , Jens Axboe , Matthew Wilcox , Christoph Hellwig , Andrew Morton , Andy Shevchenko , Dan Carpenter , "Jason A . Donenfeld" , "pedro.falcato@gmail.com" , Mateusz Guzik , "linux-mm@kvack.org" , Lorenzo Stoakes , intel-xe@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, David Airlie , Simona Vetter , Rodrigo Vivi Subject: Re: Buiild error in i915/xe Message-ID: <20250120184143.47615b21@pumpkin> In-Reply-To: <0a65de6c-74d5-4d3e-be75-0aa9ecc82da1@roeck-us.net> References: <34d53778977747f19cce2abb287bb3e6@AcuMS.aculab.com> <20250118170959.3aa56f4d@pumpkin> <29ef57a1-e4dd-4d5d-8726-f1f79c698b66@roeck-us.net> <20250118221123.5bb65e64@pumpkin> <20250119090935.7c690f85@pumpkin> <87ed0xrcb8.fsf@intel.com> <20250120111551.435176c4@pumpkin> <878qr5ras2.fsf@intel.com> <0a65de6c-74d5-4d3e-be75-0aa9ecc82da1@roeck-us.net> X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; arm-unknown-linux-gnueabihf) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Mon, 20 Jan 2025 06:15:30 -0800 Guenter Roeck wrote: > On 1/20/25 03:21, Jani Nikula wrote: > > On Mon, 20 Jan 2025, David Laight wrote: > >> On Mon, 20 Jan 2025 12:48:11 +0200 > >> Jani Nikula wrote: > >> > >>> On Sun, 19 Jan 2025, David Laight wrote: > >>>> On Sat, 18 Jan 2025 14:58:48 -0800 > >>>> Guenter Roeck wrote: > >>>> > >>>>> On 1/18/25 14:11, David Laight wrote: > >>>>>> On Sat, 18 Jan 2025 13:21:39 -0800 > >>>>>> Linus Torvalds wrote: > >>>>>> > >>>>>>> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck wrote: > >>>>>>>> > >>>>>>>> No idea why the compiler would know that the values are invalid. > >>>>>>> > >>>>>>> It's not that the compiler knows tat they are invalid, but I bet what > >>>>>>> happens is in scale() (and possibly other places that do similar > >>>>>>> checks), which does this: > >>>>>>> > >>>>>>> WARN_ON(source_min > source_max); > >>>>>>> ... > >>>>>>> source_val = clamp(source_val, source_min, source_max); > >>>>>>> > >>>>>>> and the compiler notices that the ordering comparison in the first > >>>>>>> WARN_ON() is the same as the one in clamp(), so it basically converts > >>>>>>> the logic to > >>>>>>> > >>>>>>> if (source_min > source_max) { > >>>>>>> WARN(..); > >>>>>>> /* Do the clamp() knowing that source_min > source_max */ > >>>>>>> source_val = clamp(source_val, source_min, source_max); > >>>>>>> } else { > >>>>>>> /* Do the clamp knowing that source_min <= source_max */ > >>>>>>> source_val = clamp(source_val, source_min, source_max); > >>>>>>> } > >>>>>>> > >>>>>>> (obviously I dropped the other WARN_ON in the conversion, it wasn't > >>>>>>> relevant for this case). > >>>>>>> > >>>>>>> And now that first clamp() case is done with source_min > source_max, > >>>>>>> and it triggers that build error because that's invalid. > >>>>>>> > >>>>>>> So the condition is not statically true in the *source* code, but in > >>>>>>> the "I have moved code around to combine tests" case it now *is* > >>>>>>> statically true as far as the compiler is concerned. > >>>>>> > >>>>>> Well spotted :-) > >>>>>> > >>>>>> One option would be to move the WARN_ON() below the clamp() and > >>>>>> add an OPTIMISER_HIDE_VAR(source_max) between them. > >>>>>> > >>>>>> Or do something more sensible than the WARN(). > >>>>>> Perhaps return target_min on any such errors? > >>>>>> > >>>>> > >>>>> This helps: > >>>>> > >>>>> - WARN_ON(source_min > source_max); > >>>>> - WARN_ON(target_min > target_max); > >>>>> - > >>>>> /* defensive */ > >>>>> source_val = clamp(source_val, source_min, source_max); > >>>>> > >>>>> + WARN_ON(source_min > source_max); > >>>>> + WARN_ON(target_min > target_max); > >>>> > >>>> That is a 'quick fix' ... > >>>> > >>>> Much better would be to replace the WARN() with (say): > >>>> if (target_min >= target_max) > >>>> return target_min; > >>>> if (source_min >= source_max) > >>>> return target_min + (target_max - target_min)/2; > >>>> So that the return values are actually in range (in as much as one is defined). > >>>> Note that the >= cpmparisons also remove a divide by zero. > >>> > >>> I want the loud and early warnings for clear bugs instead of > >>> "gracefully" silencing the errors only to be found through debugging > >>> user reports. > >> > >> A user isn't going to notice a WARN() - not until you tell them to look for it. > >> In any case even if you output a message you really want to return a 'sane' > >> value, who knows what effect a very out of range value is going to have. > > > > The point is, we'll catch the WARN in CI before it goes out to users. > > > > It isn't going to catch the divide by 0 error, and it obviously doesn't > catch the build problem on parisc with gcc 13.x because the CI isn't > testing it. > > How about disabling DRM_XE on architectures where it isn't supported, > matching DRM_I915 ? That'll just bite back later. As Linus spotted the compiler is just 'optimising' some code paths. It could happen for any architecture including x64. The repeated tests are basically slightly odd, although you might only expect them to be present in debug builds. An alternative would be to replace the clamp() with: if (source_val <= source_min) return target_min; if (source_val >= source_max) return target_max; David > > Thanks, > Guenter >