Re: [PATCH 1/2] lib/igt_facts: Make igt_facts_test noreturn

[Peter Senna Tschudin <peter.senna@linux.intel.com>]

Hi Pravalika,

Please see my answers below.

>>>
>>> diff --git a/lib/igt_facts.c b/lib/igt_facts.c index
>>> 2e04a7c86..bd6742065 100644
>>> --- a/lib/igt_facts.c
>>> +++ b/lib/igt_facts.c
>>> @@ -748,7 +748,7 @@ static void igt_facts_test_mark_and_sweep(struct
>> igt_list_head *head)
>>>   *
>>>   * Returns: bool indicating if the tests passed
>>>   */
>>> -void igt_facts_test(void)
>>> +__noreturn void igt_facts_test(void)
>>
>> How did you find this? Is the compiler complaining?
>>
> When am fixing the check patch errors I have seen below error 
> 
> ninja: Entering directory `build'
> [2/835] Compiling C object lib/libigt-igt_facts_c.a.p/igt_facts.c.o
> ../lib/igt_facts.c: In function ‘igt_facts_test’:
> ../lib/igt_facts.c:751:6: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn]
>   751 | void igt_facts_test(void)
>       |      ^~~~~~~~~~~~~~
> [6/6] Generating docs/testplan/intel-ci-tests with a custom command

Is your tree clean? What command line are you using? Asking because I see no
warnings when I build with:

 rm -rf build/ && meson build && ninja -C build


> 
> Then have added changes.
>>>  {
>>>  	const char *last_test = "Unit Testing";
>>>
>>> @@ -776,4 +776,6 @@ void igt_facts_test(void)
>>>  	/* Clean up the list and call igt_facts(). This should not crash */
>>>  	igt_facts_list_mark_and_sweep(&igt_facts_list_pci_gpu_head);
>>>  	igt_facts(last_test);
>>> +
>>> +	igt_exit();
>>>  }
>> Why adding igt_exit() here?
>>
>> How did you test this patch?
>>
> If we don’t explicitly specify igt_exit() I will below warning 
> 
> ninja: Entering directory `build'
> [43/999] Compiling C object lib/libigt-igt_facts_c.a.p/igt_facts.c.o
> ../lib/igt_facts.c: In function ‘igt_facts_test’:
> ../lib/igt_facts.c:779:1: warning: ‘noreturn’ function does return
>   779 | }
>       | ^
> I have compiled since these check patch errors

Curious with your command line to see these warning here too.

Compiling only is not enough testing. Please run unit testing, and a few
calls to igt_runner with facts enabled, with large test lists such as xe
bat or i915 bat, and in a RIL DUT. Compare the facts before and after
your changes to make sure nothing breaks.


> 
>> Thanks,
>>
>> Peter