Re: [PATCH v1 1/1] gcc: config: microblaze: fix cpu version check

[Michael Eager <eager@eagercon.com>]

On 10/24/23 00:01, Frager, Neal wrote:
>>>> There is a microblaze cpu version 10.0 included in versal. If the
>>>> minor version is only a single digit, then the version comparison
>>>> will fail as version 10.0 will appear as 100 compared to version
>>>> 6.00 or 8.30 which will calculate to values 600 and 830.
>>>> The issue can be seen when using the '-mcpu=10.0' option.
>>>> With this fix, versions with a single digit minor number such as
>>>> 10.0 will be calculated as greater than versions with a smaller
>>>> major version number, but with two minor version digits.
>>>> By applying this fix, several incorrect warning messages will no
>>>> longer be printed when building the versal plm application, such as
>>>> the warning message below:
>>>> warning: '-mxl-multiply-high' can be used only with '-mcpu=v6.00.a'
>>>> or greater
>>>> Signed-off-by: Neal Frager <neal.frager@amd.com>
>>>> ---
>>>>    gcc/config/microblaze/microblaze.cc | 164 +++++++++++++---------------
>>>>    1 file changed, 76 insertions(+), 88 deletions(-)
>>>
>>> Please add a test case.
>>>
>>> --
>>> Michael Eager
>>
>> Hi Michael,
>>
>> Would you mind helping me understand how to make a gcc test case for this patch?
>>
>> This patch does not change the resulting binaries of a microblaze gcc build.  The output will be the same with our without the patch, so I do not having anything in the binary itself to verify.
>>
>> All that happens is false warning messages will not be printed when building with ‘-mcpu=10.0’.  Is there a way to test for warning messages?
>>
>> In any case, please do not commit v1 of this patch.  I am going to work on making a v2 based on Mark’s feedback.
> 
>> You can create a test case which passes the -mcpu=10.0 and other options to GCC and verify that the message is not generated after the patch is applied.
> 
>> You can make all GCC warnings into errors with the "-Werror" option.
>> This means that the compile will fail if the warning is issued.
> 
>> Take a look at gcc/testsuite/gcc.target/aarch64/bti-1.c for an example of using { dg-options "<opt>" } to specify command line options.
> 
>> There is a test suite option (dg-warning) which checks that a particular source line generates a warning message, but it isn't clear whether is is possible to check that a warning is not issued.
> 
> Hi Michael,
> 
> Thanks to Mark Hatle's feedback, we have a much simpler solution to the problem.
> 
> The following change is actually all that is necessary.  Since we are just moving from
> strcasecmp to strverscmp, does v2 of the patch need to have a test case to go with it?
> 
> -#define MICROBLAZE_VERSION_COMPARE(VA,VB) strcasecmp (VA, VB)
> +#define MICROBLAZE_VERSION_COMPARE(VA,VB) strverscmp (VA, VB)
> 
> I assume there are already test cases that verify that strverscmp works correctly?

Still need a test case to verify this fix.

-- 
Michael Eager