From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.16]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 130E2187872 for ; Mon, 20 Jan 2025 10:48:22 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=198.175.65.16 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737370104; cv=none; b=cfL2ad5BLqxD186FYORUnfNOGs6xLVbWHzpWhqGRTT2K/j6Xyt+jvVeWG07tXEVkgaWg+1W7DrvKJCmAu/Kd4ZdFSVxD548y/olEsovXBy8etPXKuvDASTfQqDaNi6l2LCKqyjpCrcxfaPNGlCl0gBCNOhMlXNLQ7AmKkNXfBOI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737370104; c=relaxed/simple; bh=AhqnX1Rc1BlqEuPFl2fOCR2Q3CPQjAkoWdXUXERjTwk=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=HUyJiy+QeZzvxWX8IUDAoD/cFlzQNKwpBCOkGC49VIftqkPwDW64BzxYqYQqWh5EHp+OMCSSTyXJT0AWp6qaWz80fTIxgaXumeLq5FLodSuEFf4ERolZrWyqfjcfr8mUkwIvNDHnc6QnMGtmy0mTn/ecmjvyJWoE8I4vpdY9GXw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com; spf=pass smtp.mailfrom=intel.com; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b=IJEQDq/j; arc=none smtp.client-ip=198.175.65.16 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=intel.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="IJEQDq/j" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1737370103; x=1768906103; h=from:to:cc:subject:in-reply-to:references:date: message-id:mime-version; bh=AhqnX1Rc1BlqEuPFl2fOCR2Q3CPQjAkoWdXUXERjTwk=; b=IJEQDq/jUw2UwznZFLPTh0j5NVrtcNeXqX0QQsm4ojebZeKrjrzTNnGY kQH7yQ+9WOkIYwdgLJ/oyK33AnFyd1vfY+rB1Jg3cuqomT9qusPbGXUuU pLjSk3H8oDJsFXHi9v9ipb7jepw4pN6ig1F/bzU2iphEufFVGXwm9xfii 7s+7u1HNtzeqAt6wZeU9KZeXLW09224DddUy7bz54697L88xVY4Hoh97r 04XZqYWhbm0nMVtCDHpqOUQRvTi2b8oqoLyTGH33ZKUzWb87x/4wPEgFd 9yiUfwLVePi0S4FWCaHKNzvTzDeMCIkMBq/f8ow5cMpOg2EdQodHTeHQq w==; X-CSE-ConnectionGUID: I2NsaVf5RL67qddjiVnV2g== X-CSE-MsgGUID: IN6Q+TVbRv2U2xF5P6Kc5w== X-IronPort-AV: E=McAfee;i="6700,10204,11320"; a="37904192" X-IronPort-AV: E=Sophos;i="6.13,218,1732608000"; d="scan'208";a="37904192" Received: from orviesa008.jf.intel.com ([10.64.159.148]) by orvoesa108.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Jan 2025 02:48:22 -0800 X-CSE-ConnectionGUID: /czmvD02RK2MKgy4cJuIDg== X-CSE-MsgGUID: Z8jjReY2S7GydVJhOLE+vA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.12,224,1728975600"; d="scan'208";a="107375612" Received: from mjarzebo-mobl1.ger.corp.intel.com (HELO localhost) ([10.245.246.106]) by orviesa008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Jan 2025 02:48:14 -0800 From: Jani Nikula To: David Laight , Guenter Roeck Cc: 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 In-Reply-To: <20250119090935.7c690f85@pumpkin> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <34d53778977747f19cce2abb287bb3e6@AcuMS.aculab.com> <20250118170959.3aa56f4d@pumpkin> <29ef57a1-e4dd-4d5d-8726-f1f79c698b66@roeck-us.net> <20250118221123.5bb65e64@pumpkin> <20250119090935.7c690f85@pumpkin> Date: Mon, 20 Jan 2025 12:48:11 +0200 Message-ID: <87ed0xrcb8.fsf@intel.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain 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. BR, Jani. -- Jani Nikula, Intel