[PATCH 0/3] Fix Travis-CI / compiling with older versions of Clang

[PATCH 0/3] Fix Travis-CI / compiling with older versions of Clang

[Thomas Huth]

My last pull request enabled compiling the s390-ccw bios with Clang.
However, I only tested it with Clang version 11, and older versions
of Clang are a little bit more picky. Fix those issues now, too, so
that the corresponding job with Clang on s390x in the Travis-CI works
correctly again.

Thomas Huth (3):
  pc-bios/s390-ccw: Fix inline assembly for older versions of Clang
  pc-bios/s390-ccw/Makefile: Check more compiler flags for Clang
  pc-bios/s390-ccw: Add a proper prototype for main()

 pc-bios/s390-ccw/Makefile   | 7 ++++---
 pc-bios/s390-ccw/helper.h   | 2 +-
 pc-bios/s390-ccw/jump2ipl.c | 4 ++--
 pc-bios/s390-ccw/main.c     | 3 +--
 pc-bios/s390-ccw/menu.c     | 8 ++++----
 pc-bios/s390-ccw/s390-ccw.h | 1 +
 pc-bios/s390-ccw/virtio.c   | 2 +-
 7 files changed, 14 insertions(+), 13 deletions(-)

-- 
2.27.0

[PATCH 1/3] pc-bios/s390-ccw: Fix inline assembly for older versions of Clang

[Thomas Huth]

Clang versions before v11.0 insist on having the %rX or %cX register
names instead of just a number. Since our Travis-CI is currently
still using Clang v6.0, we have to fix this to avoid failing jobs.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/helper.h   | 2 +-
 pc-bios/s390-ccw/jump2ipl.c | 4 ++--
 pc-bios/s390-ccw/menu.c     | 8 ++++----
 pc-bios/s390-ccw/virtio.c   | 2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/pc-bios/s390-ccw/helper.h b/pc-bios/s390-ccw/helper.h
index dfcfea0ff0..3d0731c4c6 100644
--- a/pc-bios/s390-ccw/helper.h
+++ b/pc-bios/s390-ccw/helper.h
@@ -31,7 +31,7 @@ static inline void *u32toptr(uint32_t n)
 
 static inline void yield(void)
 {
-    asm volatile ("diag 0,0,0x44"
+    asm volatile ("diag %%r0,%%r0,0x44"
                   : :
                   : "memory", "cc");
 }
diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
index 73e4367e09..78f5f46533 100644
--- a/pc-bios/s390-ccw/jump2ipl.c
+++ b/pc-bios/s390-ccw/jump2ipl.c
@@ -64,8 +64,8 @@ void jump_to_IPL_code(uint64_t address)
      * We use the load normal reset to keep r15 unchanged. jump_to_IPL_2
      * can then use r15 as its stack pointer.
      */
-    asm volatile("lghi 1,1\n\t"
-                 "diag 1,1,0x308\n\t"
+    asm volatile("lghi %%r1,1\n\t"
+                 "diag %%r1,%%r1,0x308\n\t"
                  : : : "1", "memory");
     panic("\n! IPL returns !\n");
 }
diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
index de8260a5d6..d601952d3e 100644
--- a/pc-bios/s390-ccw/menu.c
+++ b/pc-bios/s390-ccw/menu.c
@@ -36,9 +36,9 @@ static inline void enable_clock_int(void)
     uint64_t tmp = 0;
 
     asm volatile(
-        "stctg      0,0,%0\n"
+        "stctg      %%c0,%%c0,%0\n"
         "oi         6+%0, 0x8\n"
-        "lctlg      0,0,%0"
+        "lctlg      %%c0,%%c0,%0"
         : : "Q" (tmp) : "memory"
     );
 }
@@ -48,9 +48,9 @@ static inline void disable_clock_int(void)
     uint64_t tmp = 0;
 
     asm volatile(
-        "stctg      0,0,%0\n"
+        "stctg      %%c0,%%c0,%0\n"
         "ni         6+%0, 0xf7\n"
-        "lctlg      0,0,%0"
+        "lctlg      %%c0,%%c0,%0"
         : : "Q" (tmp) : "memory"
     );
 }
diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
index ab49840db8..5d2c6e3381 100644
--- a/pc-bios/s390-ccw/virtio.c
+++ b/pc-bios/s390-ccw/virtio.c
@@ -54,7 +54,7 @@ static long kvm_hypercall(unsigned long nr, unsigned long param1,
     register ulong r_param3 asm("4") = param3;
     register long retval asm("2");
 
-    asm volatile ("diag 2,4,0x500"
+    asm volatile ("diag %%r2,%%r4,0x500"
                   : "=d" (retval)
                   : "d" (r_nr), "0" (r_param1), "r"(r_param2), "d"(r_param3)
                   : "memory", "cc");
-- 
2.27.0

Re: [PATCH 1/3] pc-bios/s390-ccw: Fix inline assembly for older versions of Clang

[Philippe Mathieu-Daudé]

On 5/12/21 7:15 PM, Thomas Huth wrote:
> Clang versions before v11.0 insist on having the %rX or %cX register
> names instead of just a number. Since our Travis-CI is currently
> still using Clang v6.0, we have to fix this to avoid failing jobs.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/helper.h   | 2 +-
>  pc-bios/s390-ccw/jump2ipl.c | 4 ++--
>  pc-bios/s390-ccw/menu.c     | 8 ++++----
>  pc-bios/s390-ccw/virtio.c   | 2 +-
>  4 files changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 1/3] pc-bios/s390-ccw: Fix inline assembly for older versions of Clang

[Cornelia Huck]

On Wed, 12 May 2021 19:15:48 +0200
Thomas Huth <thuth@redhat.com> wrote:

> Clang versions before v11.0 insist on having the %rX or %cX register
> names instead of just a number. Since our Travis-CI is currently
> still using Clang v6.0, we have to fix this to avoid failing jobs.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/helper.h   | 2 +-
>  pc-bios/s390-ccw/jump2ipl.c | 4 ++--
>  pc-bios/s390-ccw/menu.c     | 8 ++++----
>  pc-bios/s390-ccw/virtio.c   | 2 +-
>  4 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/helper.h b/pc-bios/s390-ccw/helper.h
> index dfcfea0ff0..3d0731c4c6 100644
> --- a/pc-bios/s390-ccw/helper.h
> +++ b/pc-bios/s390-ccw/helper.h
> @@ -31,7 +31,7 @@ static inline void *u32toptr(uint32_t n)
>  
>  static inline void yield(void)
>  {
> -    asm volatile ("diag 0,0,0x44"
> +    asm volatile ("diag %%r0,%%r0,0x44"
>                    : :
>                    : "memory", "cc");
>  }

Sigh, this really looks uglier, but if it pleases the compiler...

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH 1/3] pc-bios/s390-ccw: Fix inline assembly for older versions of Clang

[Philippe Mathieu-Daudé]

On 5/17/21 6:14 PM, Cornelia Huck wrote:
> On Wed, 12 May 2021 19:15:48 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> Clang versions before v11.0 insist on having the %rX or %cX register
>> names instead of just a number. Since our Travis-CI is currently
>> still using Clang v6.0, we have to fix this to avoid failing jobs.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  pc-bios/s390-ccw/helper.h   | 2 +-
>>  pc-bios/s390-ccw/jump2ipl.c | 4 ++--
>>  pc-bios/s390-ccw/menu.c     | 8 ++++----
>>  pc-bios/s390-ccw/virtio.c   | 2 +-
>>  4 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/helper.h b/pc-bios/s390-ccw/helper.h
>> index dfcfea0ff0..3d0731c4c6 100644
>> --- a/pc-bios/s390-ccw/helper.h
>> +++ b/pc-bios/s390-ccw/helper.h
>> @@ -31,7 +31,7 @@ static inline void *u32toptr(uint32_t n)
>>  
>>  static inline void yield(void)
>>  {
>> -    asm volatile ("diag 0,0,0x44"
>> +    asm volatile ("diag %%r0,%%r0,0x44"
>>                    : :
>>                    : "memory", "cc");
>>  }
> 
> Sigh, this really looks uglier, but if it pleases the compiler...

Personally I find it easier to read, it makes obvious we are
accessing a register, not using an immediate value.

[PATCH 2/3] pc-bios/s390-ccw/Makefile: Check more compiler flags for Clang

[Thomas Huth]

Older versions of Clang do not like -fno-delete-null-pointer-checks
or -msoft-float. So let's add a proper check for those.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/Makefile | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
index cee9d2c63b..97008d94c9 100644
--- a/pc-bios/s390-ccw/Makefile
+++ b/pc-bios/s390-ccw/Makefile
@@ -31,10 +31,11 @@ OBJECTS = start.o main.o bootmap.o jump2ipl.o sclp.o menu.o \
 
 QEMU_CFLAGS := -Wall $(filter -W%, $(QEMU_CFLAGS))
 QEMU_CFLAGS += $(call cc-option,-Werror $(QEMU_CFLAGS),-Wno-stringop-overflow)
-QEMU_CFLAGS += -ffreestanding -fno-delete-null-pointer-checks -fno-common -fPIE
+QEMU_CFLAGS += -ffreestanding -fno-common -fPIE
 QEMU_CFLAGS += -fwrapv -fno-strict-aliasing -fno-asynchronous-unwind-tables
-QEMU_CFLAGS += $(call cc-option, $(QEMU_CFLAGS), -fno-stack-protector)
-QEMU_CFLAGS += -msoft-float
+QEMU_CFLAGS += $(call cc-option,-Werror $(QEMU_CFLAGS),-fno-delete-null-pointer-checks)
+QEMU_CFLAGS += $(call cc-option,-Werror $(QEMU_CFLAGS),-fno-stack-protector)
+QEMU_CFLAGS += $(call cc-option,-Werror $(QEMU_CFLAGS),-msoft-float)
 QEMU_CFLAGS += $(call cc-option, $(QEMU_CFLAGS),-march=z900,-march=z10)
 QEMU_CFLAGS += -std=gnu99
 LDFLAGS += -Wl,-pie -nostdlib
-- 
2.27.0

Re: [PATCH 2/3] pc-bios/s390-ccw/Makefile: Check more compiler flags for Clang

[Philippe Mathieu-Daudé]

+Richard

On 5/12/21 7:15 PM, Thomas Huth wrote:
> Older versions of Clang do not like -fno-delete-null-pointer-checks
> or -msoft-float. So let's add a proper check for those.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/Makefile | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
> index cee9d2c63b..97008d94c9 100644
> --- a/pc-bios/s390-ccw/Makefile
> +++ b/pc-bios/s390-ccw/Makefile
> @@ -31,10 +31,11 @@ OBJECTS = start.o main.o bootmap.o jump2ipl.o sclp.o menu.o \
>  
>  QEMU_CFLAGS := -Wall $(filter -W%, $(QEMU_CFLAGS))
>  QEMU_CFLAGS += $(call cc-option,-Werror $(QEMU_CFLAGS),-Wno-stringop-overflow)
> -QEMU_CFLAGS += -ffreestanding -fno-delete-null-pointer-checks -fno-common -fPIE
> +QEMU_CFLAGS += -ffreestanding -fno-common -fPIE

OK

>  QEMU_CFLAGS += -fwrapv -fno-strict-aliasing -fno-asynchronous-unwind-tables
> -QEMU_CFLAGS += $(call cc-option, $(QEMU_CFLAGS), -fno-stack-protector)
> -QEMU_CFLAGS += -msoft-float
> +QEMU_CFLAGS += $(call cc-option,-Werror $(QEMU_CFLAGS),-fno-delete-null-pointer-checks)
> +QEMU_CFLAGS += $(call cc-option,-Werror $(QEMU_CFLAGS),-fno-stack-protector)

OK

> +QEMU_CFLAGS += $(call cc-option,-Werror $(QEMU_CFLAGS),-msoft-float)

For this one I don't understand clang... How to enforce no floating
point code generation?

>  QEMU_CFLAGS += $(call cc-option, $(QEMU_CFLAGS),-march=z900,-march=z10)
>  QEMU_CFLAGS += -std=gnu99
>  LDFLAGS += -Wl,-pie -nostdlib
> 

[PATCH 3/3] pc-bios/s390-ccw: Add a proper prototype for main()

[Thomas Huth]

Older versions of Clang complain if there is no prototype for main().
Add one, and while we're at it, make sure that we use the same type
for main.c and netmain.c - since the return value does not matter,
declare the return type of main() as "void".

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/main.c     | 3 +--
 pc-bios/s390-ccw/s390-ccw.h | 1 +
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 5d2b7ba94d..835341457d 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -281,7 +281,7 @@ static void probe_boot_device(void)
     sclp_print("Could not find a suitable boot device (none specified)\n");
 }
 
-int main(void)
+void main(void)
 {
     sclp_setup();
     css_setup();
@@ -294,5 +294,4 @@ int main(void)
     }
 
     panic("Failed to load OS from hard disk\n");
-    return 0; /* make compiler happy */
 }
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index 79db69ff54..b88e0550ab 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -57,6 +57,7 @@ void write_subsystem_identification(void);
 void write_iplb_location(void);
 extern char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
 unsigned int get_loadparm_index(void);
+void main(void);
 
 /* sclp.c */
 void sclp_print(const char *string);
-- 
2.27.0

Re: [PATCH 3/3] pc-bios/s390-ccw: Add a proper prototype for main()

[Philippe Mathieu-Daudé]

On 5/12/21 7:15 PM, Thomas Huth wrote:
> Older versions of Clang complain if there is no prototype for main().
> Add one, and while we're at it, make sure that we use the same type
> for main.c and netmain.c - since the return value does not matter,
> declare the return type of main() as "void".
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/main.c     | 3 +--
>  pc-bios/s390-ccw/s390-ccw.h | 1 +
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 5d2b7ba94d..835341457d 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -281,7 +281,7 @@ static void probe_boot_device(void)
>      sclp_print("Could not find a suitable boot device (none specified)\n");
>  }
>  
> -int main(void)
> +void main(void)
>  {
>      sclp_setup();
>      css_setup();
> @@ -294,5 +294,4 @@ int main(void)
>      }
>  
>      panic("Failed to load OS from hard disk\n");
> -    return 0; /* make compiler happy */
>  }
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> index 79db69ff54..b88e0550ab 100644
> --- a/pc-bios/s390-ccw/s390-ccw.h
> +++ b/pc-bios/s390-ccw/s390-ccw.h
> @@ -57,6 +57,7 @@ void write_subsystem_identification(void);
>  void write_iplb_location(void);
>  extern char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
>  unsigned int get_loadparm_index(void);
> +void main(void);

Can we keep the forward declaration in the source?

Re: [PATCH 3/3] pc-bios/s390-ccw: Add a proper prototype for main()

[Thomas Huth]

On 12/05/2021 21.54, Philippe Mathieu-Daudé wrote:
> On 5/12/21 7:15 PM, Thomas Huth wrote:
>> Older versions of Clang complain if there is no prototype for main().
>> Add one, and while we're at it, make sure that we use the same type
>> for main.c and netmain.c - since the return value does not matter,
>> declare the return type of main() as "void".
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   pc-bios/s390-ccw/main.c     | 3 +--
>>   pc-bios/s390-ccw/s390-ccw.h | 1 +
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
>> index 5d2b7ba94d..835341457d 100644
>> --- a/pc-bios/s390-ccw/main.c
>> +++ b/pc-bios/s390-ccw/main.c
>> @@ -281,7 +281,7 @@ static void probe_boot_device(void)
>>       sclp_print("Could not find a suitable boot device (none specified)\n");
>>   }
>>   
>> -int main(void)
>> +void main(void)
>>   {
>>       sclp_setup();
>>       css_setup();
>> @@ -294,5 +294,4 @@ int main(void)
>>       }
>>   
>>       panic("Failed to load OS from hard disk\n");
>> -    return 0; /* make compiler happy */
>>   }
>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>> index 79db69ff54..b88e0550ab 100644
>> --- a/pc-bios/s390-ccw/s390-ccw.h
>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>> @@ -57,6 +57,7 @@ void write_subsystem_identification(void);
>>   void write_iplb_location(void);
>>   extern char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
>>   unsigned int get_loadparm_index(void);
>> +void main(void);
> 
> Can we keep the forward declaration in the source?

There are two main() functions, one in main.c and one in netmain.c, that's 
why I think it's better to declare the prototype only in one place, i.e. in 
a header?

  Thomas

Re: [PATCH 3/3] pc-bios/s390-ccw: Add a proper prototype for main()

[Philippe Mathieu-Daudé]

On 5/14/21 12:09 PM, Thomas Huth wrote:
> On 12/05/2021 21.54, Philippe Mathieu-Daudé wrote:
>> On 5/12/21 7:15 PM, Thomas Huth wrote:
>>> Older versions of Clang complain if there is no prototype for main().
>>> Add one, and while we're at it, make sure that we use the same type
>>> for main.c and netmain.c - since the return value does not matter,
>>> declare the return type of main() as "void".
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   pc-bios/s390-ccw/main.c     | 3 +--
>>>   pc-bios/s390-ccw/s390-ccw.h | 1 +
>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
>>> index 5d2b7ba94d..835341457d 100644
>>> --- a/pc-bios/s390-ccw/main.c
>>> +++ b/pc-bios/s390-ccw/main.c
>>> @@ -281,7 +281,7 @@ static void probe_boot_device(void)
>>>       sclp_print("Could not find a suitable boot device (none
>>> specified)\n");
>>>   }
>>>   -int main(void)
>>> +void main(void)
>>>   {
>>>       sclp_setup();
>>>       css_setup();
>>> @@ -294,5 +294,4 @@ int main(void)
>>>       }
>>>         panic("Failed to load OS from hard disk\n");
>>> -    return 0; /* make compiler happy */
>>>   }
>>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>>> index 79db69ff54..b88e0550ab 100644
>>> --- a/pc-bios/s390-ccw/s390-ccw.h
>>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>>> @@ -57,6 +57,7 @@ void write_subsystem_identification(void);
>>>   void write_iplb_location(void);
>>>   extern char stack[PAGE_SIZE * 8]
>>> __attribute__((__aligned__(PAGE_SIZE)));
>>>   unsigned int get_loadparm_index(void);
>>> +void main(void);
>>
>> Can we keep the forward declaration in the source?
> 
> There are two main() functions, one in main.c and one in netmain.c,
> that's why I think it's better to declare the prototype only in one
> place, i.e. in a header?

Since it is a kludge to make the compiler happy, I'd rather keep
it local, but I don't mind much :)

Re: [PATCH 3/3] pc-bios/s390-ccw: Add a proper prototype for main()

[Cornelia Huck]

On Wed, 12 May 2021 19:15:50 +0200
Thomas Huth <thuth@redhat.com> wrote:

> Older versions of Clang complain if there is no prototype for main().
> Add one, and while we're at it, make sure that we use the same type
> for main.c and netmain.c - since the return value does not matter,
> declare the return type of main() as "void".
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/main.c     | 3 +--
>  pc-bios/s390-ccw/s390-ccw.h | 1 +
>  2 files changed, 2 insertions(+), 2 deletions(-)

It's probably not strictly needed for both to have the same prototype,
but this looks sane to me.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>