From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.15]) (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 E165B1BA89C for ; Mon, 20 Jan 2025 11:21:27 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=192.198.163.15 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737372089; cv=none; b=SlhpqkW8MpHIrqYsJxogW+zJjlrptlyPEY/uF+/FHv5E3O63QVDaIWnv+HFVDTS8GJkf+dpFrL+bTWLaujuCg1coJ8Q47HNQu2UkZsQxmTZcK3JCJDIyC7MB0s0BgPAlRz9iVcGccbH2F+D5ud+FvyDhNeDMnS22oqal8CfcYx0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737372089; c=relaxed/simple; bh=Bqm7nHO1/aCxnCIrdM+FoX3RzC8TchksMVJHfy7cAHU=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=Hfs6RdFX/24MUPC4iZPZSbMDw1HqvAsuxgzEcuZtD7rARkGprphOPO1BZjYjJUHqLAW0QjoYrYZLSKNWsbpCUzkzKv9ViOYtKy8ekDVrYv7o3GVdZPIIfLTSo0uDD4huHJ0B094c4ljPlS7vz44pF6zc2h1F5vRzFC26wUduSkY= 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=HM1dDPBB; arc=none smtp.client-ip=192.198.163.15 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="HM1dDPBB" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1737372087; x=1768908087; h=from:to:cc:subject:in-reply-to:references:date: message-id:mime-version; bh=Bqm7nHO1/aCxnCIrdM+FoX3RzC8TchksMVJHfy7cAHU=; b=HM1dDPBBsIYZdWlPR41rZxVvLDh6sPONg3bGF+D5CAshCIup077GKcFU JdXAfGwHn9RDgfWifj2WgwElNsnC/bJkXkIijLwNZ28Rza6JMFELjETVh Utp+GKLFCn29XKTVirb4h4QRjodOTrlw4mQfMJj0MXd3TI+oPhpUqEA1F rMXfyUEYZJYrgXp7zEijnynil1SQkWYWvreh63VHJkR0yQMOttlE/XRkg Ht18KTgcZKWZ1YE3r/0H7FcoRhCoebto8fQJ0MrMlGYl4MlTTANJwrKI9 RzFe9g9TPFC9MUYvs7SJv4MLBSnaJFxA/hXpCrWS9nKFZ9/5FNxWrB0u0 w==; X-CSE-ConnectionGUID: UIYTg6JZR82MOiVjeJFnhg== X-CSE-MsgGUID: 54QasxdtRUSKytB7oG7qqg== X-IronPort-AV: E=McAfee;i="6700,10204,11320"; a="37917865" X-IronPort-AV: E=Sophos;i="6.13,218,1732608000"; d="scan'208";a="37917865" Received: from fmviesa010.fm.intel.com ([10.60.135.150]) by fmvoesa109.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Jan 2025 03:21:27 -0800 X-CSE-ConnectionGUID: MvDLfc5CTomhxALmG/Ajug== X-CSE-MsgGUID: SntxViPMT/+ezMNA1rOSfQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.13,218,1732608000"; d="scan'208";a="107017049" Received: from mjarzebo-mobl1.ger.corp.intel.com (HELO localhost) ([10.245.246.106]) by fmviesa010-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Jan 2025 03:21:20 -0800 From: Jani Nikula To: David Laight Cc: Guenter Roeck , 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: <20250120111551.435176c4@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> <87ed0xrcb8.fsf@intel.com> <20250120111551.435176c4@pumpkin> Date: Mon, 20 Jan 2025 13:21:17 +0200 Message-ID: <878qr5ras2.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 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. BR, Jani. > > David > > -- Jani Nikula, Intel