[PATCH] drm/xe: Fix warning on unreachable statement

[Lucas De Marchi]

On Sun, Jul 21, 2024 at 12:25:34PM GMT, Nathan Chancellor wrote:
>Hi Lucas,
>
>On Fri, Jul 19, 2024 at 12:15:34PM -0700, Lucas De Marchi wrote:
>> eu_type_to_str() relies on -Wswitch to warn (and -Werror) to make sure
>> it handles all enum values. However it's perfectly legal to pass an int
>> to that function so in the end that function may happen to return
>> nothing. A smart compiler could notice eu_type is never assigned to
>> anything other than those values.
>
>Well that assignment happens in load_eu_mask(), which is obviously
>called before xe_gt_topology_dump() in xe_gt_topology_init(), but I
>don't see how the compiler could assume that eu_type is always one of
>those two values when xe_gt_topology_dump() is not static and could be
>called from anywhere (i.e., when eu_type is potentially some other
>value)? I might be missing something though.

True, it's not an easy check by the compiler/linker as I originally thought
as it depends on some additional knowledge:

	- eu_type is only ever assigned in one place
	- it's never allocated from the stack
	- it's only ever used in a zero-initialized object and 0 is a
	  valid value.


>
>> Trying to reproduce this issue, none of gcc-9, gcc-10 and gcc-13
>> triggered for me, but this was reported in a different system with
>> gcc-10:
>>
>> 	drivers/gpu/drm/xe/xe.o: warning: objtool: xe_gt_topology_dump() falls through to next function xe_gt_topology_init()
>
>I have been seeing similar warnings with clang as well, such as:
>
>  drivers/gpu/drm/xe/xe.o: warning: objtool: xe_gt_topology_dump+0x77: sibling call from callable instruction with modified stack frame
>
>  drivers/gpu/drm/xe/xe.o: warning: objtool: xe_gt_topology_dump() falls through to next function xe_dss_mask_group_ffs()
>
>  drivers/gpu/drm/xe/xe.o: warning: objtool: xe_gt_topology_dump+0x77: can't find jump dest instruction at .text.xe_gt_topology_dump+0xc0
>
>The final warning happens when LTO and CFI are enabled and I see a CFI
>failure that appears to be a result of that.
>
>[  +0.008116] Missing ENDBR: __cfi_init_module+0x0/0x10 [xe]
>[  +0.000226] ------------[ cut here ]------------
>[  +0.000001] kernel BUG at arch/x86/kernel/cet.c:102!
>[  +0.000014] Oops: invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
>[  +0.000004] CPU: 3 PID: 335 Comm: (udev-worker) Not tainted 6.10.0-rc3-debug-01858-g7108b4a589cd #1 6163aa9290b9c012af9bc3cd405d04758721ac11
>[  +0.000005] Hardware name: AZW MINI S/MINI S, BIOS ADLNV105 12/12/2023
>...
>[  +0.000002]  ? __cfi_init_module+0x10/0x10 [xe 257ff34a29c86b7d55c1c838833ff13eec5c3393]
>[  +0.000167]  ? xe_hw_fence_module_init+0x40/0x40 [xe 257ff34a29c86b7d55c1c838833ff13eec5c3393]
>[  +0.000171]  ? __cfi_init_module+0x10/0x10 [xe 257ff34a29c86b7d55c1c838833ff13eec5c3393]
>[  +0.000174]  ? do_one_initcall+0x147/0x350
>
>> Since that is not really possible, just take the simple approach and
>> return NULL.
>
>Indeed, bare unreachable() is generally considered harmful and can introduce
>undefined behavior. Commits
>
>  d652d5f1eeeb ("drm/edid: fix objtool warning in drm_cvt_modes()")
>
>and
>
>  3764647b255a ("bcachefs: Remove undefined behavior in bch2_dev_buckets_reserved()")
>
>come to mind as instances like this.
>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>
>This change resolves all the issues I have noted above, so thank you!
>
>Reviewed-by: Nathan Chancellor <nathan at kernel.org>
>Tested-by: Nathan Chancellor <nathan at kernel.org>

thanks. I will amend the commit message with additional note on clang
and push to drm-xe-next.

Lucas De Marchi

>
>> ---
>>  drivers/gpu/drm/xe/xe_gt_topology.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gt_topology.c b/drivers/gpu/drm/xe/xe_gt_topology.c
>> index 5a1559edf3e9..0662f71c6ede 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_topology.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_topology.c
>> @@ -233,7 +233,7 @@ static const char *eu_type_to_str(enum xe_gt_eu_type eu_type)
>>  		return "simd8";
>>  	}
>>
>> -	unreachable();
>> +	return NULL;
>>  }
>>
>>  void
>> --
>> 2.43.0
>>