[PATCH] Fix C compatibility issue in mini-os

Florian Weimer posted 1 patch 4 months, 3 weeks ago
Failed in applying to current master (apply log)
[PATCH] Fix C compatibility issue in mini-os
Posted by Florian Weimer 4 months, 3 weeks ago
The cc-option check always fails (that, it picks the second option
unconditionally) if the compiler does not support implicit conversion
from integers to pointers.  Just drop the initialization because it
seems unnecessary in this context.

Related to:

  <https://fedoraproject.org/wiki/Changes/PortingToModernC>
  <https://fedoraproject.org/wiki/Toolchain/PortingToModernC>

Signed-off-by: Florian Weimer <fweimer@redhat.com>

diff --git a/Config.mk b/Config.mk
index f2d1f0a..b7c9887 100644
--- a/Config.mk
+++ b/Config.mk
@@ -21,7 +21,7 @@ endef
 # of which would indicate an "unrecognized command-line option" warning/error.
 #
 # Usage: cflags-y += $(call cc-option,$(CC),-march=winchip-c6,-march=i586)
-cc-option = $(shell if test -z "`echo 'void*p=1;' | \
+cc-option = $(shell if test -z "`echo 'void*p;' | \
               $(1) $(2) -c -o /dev/null -x c - 2>&1 | grep -- $(2) -`"; \
               then echo "$(2)"; else echo "$(3)"; fi ;)
Re: [PATCH] Fix C compatibility issue in mini-os
Posted by Jan Beulich 4 months, 3 weeks ago
On 15.12.2023 12:59, Florian Weimer wrote:
> The cc-option check always fails (that, it picks the second option
> unconditionally) if the compiler does not support implicit conversion
> from integers to pointers.  Just drop the initialization because it
> seems unnecessary in this context.

Did you pay attention to ...

> --- a/Config.mk
> +++ b/Config.mk
> @@ -21,7 +21,7 @@ endef
>  # of which would indicate an "unrecognized command-line option" warning/error.

... the comment the tail of which is visible here?

Jan

>  #
>  # Usage: cflags-y += $(call cc-option,$(CC),-march=winchip-c6,-march=i586)
> -cc-option = $(shell if test -z "`echo 'void*p=1;' | \
> +cc-option = $(shell if test -z "`echo 'void*p;' | \
>                $(1) $(2) -c -o /dev/null -x c - 2>&1 | grep -- $(2) -`"; \
>                then echo "$(2)"; else echo "$(3)"; fi ;)
>  
> 
>
Re: [PATCH] Fix C compatibility issue in mini-os
Posted by Florian Weimer 4 months, 3 weeks ago
* Jan Beulich:

> On 15.12.2023 12:59, Florian Weimer wrote:
>> The cc-option check always fails (that, it picks the second option
>> unconditionally) if the compiler does not support implicit conversion
>> from integers to pointers.  Just drop the initialization because it
>> seems unnecessary in this context.
>
> Did you pay attention to ...
>
>> --- a/Config.mk
>> +++ b/Config.mk
>> @@ -21,7 +21,7 @@ endef
>>  # of which would indicate an "unrecognized command-line option" warning/error.
>
> ... the comment the tail of which is visible here?

I didn't investigate how the mechanics of the selection are
accomplished.  I was so happy that I finally found the source of the
int-conversion error and got a bit carried away.

Looking at the shell script fragment:

>>  # Usage: cflags-y += $(call cc-option,$(CC),-march=winchip-c6,-march=i586)
>> -cc-option = $(shell if test -z "`echo 'void*p=1;' | \
>> +cc-option = $(shell if test -z "`echo 'void*p;' | \
>>                $(1) $(2) -c -o /dev/null -x c - 2>&1 | grep -- $(2) -`"; \
>>                then echo "$(2)"; else echo "$(3)"; fi ;)

I can see that the exit status of the compiler does not matter, so the
patch will not make a difference.

I'll record this as a false positive.

Thanks,
Florian
Re: [PATCH] Fix C compatibility issue in mini-os
Posted by Jan Beulich 4 months, 3 weeks ago
On 15.12.2023 13:23, Florian Weimer wrote:
> * Jan Beulich:
> 
>> On 15.12.2023 12:59, Florian Weimer wrote:
>>> The cc-option check always fails (that, it picks the second option
>>> unconditionally) if the compiler does not support implicit conversion
>>> from integers to pointers.  Just drop the initialization because it
>>> seems unnecessary in this context.
>>
>> Did you pay attention to ...
>>
>>> --- a/Config.mk
>>> +++ b/Config.mk
>>> @@ -21,7 +21,7 @@ endef
>>>  # of which would indicate an "unrecognized command-line option" warning/error.
>>
>> ... the comment the tail of which is visible here?
> 
> I didn't investigate how the mechanics of the selection are
> accomplished.  I was so happy that I finally found the source of the
> int-conversion error and got a bit carried away.
> 
> Looking at the shell script fragment:
> 
>>>  # Usage: cflags-y += $(call cc-option,$(CC),-march=winchip-c6,-march=i586)
>>> -cc-option = $(shell if test -z "`echo 'void*p=1;' | \
>>> +cc-option = $(shell if test -z "`echo 'void*p;' | \
>>>                $(1) $(2) -c -o /dev/null -x c - 2>&1 | grep -- $(2) -`"; \
>>>                then echo "$(2)"; else echo "$(3)"; fi ;)
> 
> I can see that the exit status of the compiler does not matter, so the
> patch will not make a difference.

An option may be to use Xen's (newer) variant

cc-option = $(shell if $(1) $(2:-Wno-%=-W%) -Werror -c -o /dev/null -x c /dev/null >/dev/null 2>&1; \
              then echo "$(2)"; else echo "$(3)"; fi ;)

here as well (omitted all commentary). Cc-ing maintainers for possible
input.

Jan
Re: [PATCH] Fix C compatibility issue in mini-os
Posted by Juergen Gross 4 months, 3 weeks ago
On 15.12.23 13:29, Jan Beulich wrote:
> On 15.12.2023 13:23, Florian Weimer wrote:
>> * Jan Beulich:
>>
>>> On 15.12.2023 12:59, Florian Weimer wrote:
>>>> The cc-option check always fails (that, it picks the second option
>>>> unconditionally) if the compiler does not support implicit conversion
>>>> from integers to pointers.  Just drop the initialization because it
>>>> seems unnecessary in this context.
>>>
>>> Did you pay attention to ...
>>>
>>>> --- a/Config.mk
>>>> +++ b/Config.mk
>>>> @@ -21,7 +21,7 @@ endef
>>>>   # of which would indicate an "unrecognized command-line option" warning/error.
>>>
>>> ... the comment the tail of which is visible here?
>>
>> I didn't investigate how the mechanics of the selection are
>> accomplished.  I was so happy that I finally found the source of the
>> int-conversion error and got a bit carried away.
>>
>> Looking at the shell script fragment:
>>
>>>>   # Usage: cflags-y += $(call cc-option,$(CC),-march=winchip-c6,-march=i586)
>>>> -cc-option = $(shell if test -z "`echo 'void*p=1;' | \
>>>> +cc-option = $(shell if test -z "`echo 'void*p;' | \
>>>>                 $(1) $(2) -c -o /dev/null -x c - 2>&1 | grep -- $(2) -`"; \
>>>>                 then echo "$(2)"; else echo "$(3)"; fi ;)
>>
>> I can see that the exit status of the compiler does not matter, so the
>> patch will not make a difference.
> 
> An option may be to use Xen's (newer) variant
> 
> cc-option = $(shell if $(1) $(2:-Wno-%=-W%) -Werror -c -o /dev/null -x c /dev/null >/dev/null 2>&1; \
>                then echo "$(2)"; else echo "$(3)"; fi ;)
> 
> here as well (omitted all commentary). Cc-ing maintainers for possible
> input.

There is no urgent need for that, as in Mini-OS I'm not aware of any use case
to test for a "-W..." option, but the approach is really cleaner IMO.


Juergen