Skip to contentSkip to conversation

Clang patches

PATCH In review Blue Swirl 3 participants 10 messages Aug 1, 2012 – Aug 2, 2012

Subsystems (2)
Networking Arm/Zynq architecture
lore
Share & tools
lore

Blue Swirl

author
#0 lore raw

v2 cover Clang patches

With this patch set, I'm able to compile AREG0 free softmmu targets.

v1->v2: redo QAPI change (#3), new patches 5 and 6.

Blue Swirl (6):
sparc: fix floppy TC line setup
sparc: fix expression with uninitialized initial value
qapi: avoid reserved keywords
user: fix accidental AREG0 use
xilinx_axienet: avoid useless self-assignment
scsi-bus: remove overlapping entry

 hw/scsi-bus.c       |    1 -
 hw/sun4m.c          |   18 +++++++++++++-----
 hw/xilinx_axienet.c |    1 -
 linux-user/signal.c |    8 +++-----
 net/slirp.c         |    6 +++---
 scripts/qapi.py     |   16 ++++++++++++++++
 user-exec.c         |   17 ++++++++++++-----
 7 files changed, 47 insertions(+), 20 deletions(-)
Signature · 1 line
1.7.2.5
author
#1 lore raw

PATCH v2 1/6 sparc: fix floppy TC line setup

The qemu_irq for Terminal Count (TC) line between FDC and Slavio misc device was created only after use, spotted by Clang compiler. Also, it was not created if the FDC didn't exist.

Rearrange code to fix order. Always create the TC line.

Signed-off-by Blue Swirl <[email protected]>
---
 hw/sun4m.c |   18 +++++++++++++-----
 1 files changed, 13 insertions(+), 5 deletions(-)
hw/sun4m.c +13 -5
index a959261..0f909b5 100644
@@ -832,6 +832,10 @@ static void cpu_devinit(const char *cpu_model, unsigned int id,
     env->prom_addr = prom_addr;
 }
 
+static void dummy_fdc_tc(void *opaque, int irq, int level)
+{
+}
+
 static void sun4m_hw_init(const struct sun4m_hwdef *hwdef, ram_addr_t RAM_size,
                           const char *boot_device,
                           const char *kernel_filename,

@@ -942,9 +946,6 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef, ram_addr_t RAM_size,
               serial_hds[0], serial_hds[1], ESCC_CLOCK, 1);
 
     cpu_halt = qemu_allocate_irqs(cpu_halt_signal, NULL, 1);
-    slavio_misc_init(hwdef->slavio_base, hwdef->aux1_base, hwdef->aux2_base,
-                     slavio_irq[30], fdc_tc);
-
     if (hwdef->apc_base) {
         apc_init(hwdef->apc_base, cpu_halt[0]);
     }

@@ -955,8 +956,13 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef, ram_addr_t RAM_size,
         fd[0] = drive_get(IF_FLOPPY, 0, 0);
         sun4m_fdctrl_init(slavio_irq[22], hwdef->fd_base, fd,
                           &fdc_tc);
+    } else {
+        fdc_tc = *qemu_allocate_irqs(dummy_fdc_tc, NULL, 1);
     }
 
+    slavio_misc_init(hwdef->slavio_base, hwdef->aux1_base, hwdef->aux2_base,
+                     slavio_irq[30], fdc_tc);
+
     if (drive_get_max_bus(IF_SCSI) > 0) {
         fprintf(stderr, "qemu: too many SCSI bus\n");
         exit(1);

@@ -1772,16 +1778,18 @@ static void sun4c_hw_init(const struct sun4c_hwdef *hwdef, ram_addr_t RAM_size,
               slavio_irq[1], serial_hds[0], serial_hds[1],
               ESCC_CLOCK, 1);
 
-    slavio_misc_init(0, hwdef->aux1_base, 0, slavio_irq[1], fdc_tc);
-
     if (hwdef->fd_base != (target_phys_addr_t)-1) {
         /* there is zero or one floppy drive */
         memset(fd, 0, sizeof(fd));
         fd[0] = drive_get(IF_FLOPPY, 0, 0);
         sun4m_fdctrl_init(slavio_irq[1], hwdef->fd_base, fd,
                           &fdc_tc);
+    } else {
+        fdc_tc = *qemu_allocate_irqs(dummy_fdc_tc, NULL, 1);
     }
 
+    slavio_misc_init(0, hwdef->aux1_base, 0, slavio_irq[1], fdc_tc);
+
     if (drive_get_max_bus(IF_SCSI) > 0) {
         fprintf(stderr, "qemu: too many SCSI bus\n");
         exit(1);
Signature · 1 line
1.7.2.5
author
#2 lore raw

PATCH v2 2/6 sparc: fix expression with uninitialized initial value

In reply to Blue Swirl

err was uninitialized, it's not OK to use |=. Spotted by Clang compiler.

Fix by implementing the earlier statement which initializes the variable.

Signed-off-by Blue Swirl <[email protected]>
---
 linux-user/signal.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)
linux-user/signal.c +3 -5
index 9be5ac0..7869147 100644
@@ -1844,7 +1844,7 @@ typedef struct {
 } __siginfo_t;
 
 typedef struct {
-        unsigned   long si_float_regs [32];
+        abi_ulong       si_float_regs[32];
         unsigned   long si_fsr;
         unsigned   long si_fpqdepth;
         struct {

@@ -2056,11 +2056,9 @@ restore_fpu_state(CPUSPARCState *env, qemu_siginfo_fpu_t *fpu)
                 return -EFAULT;
 #endif
 
-#if 0
         /* XXX: incorrect */
-        err = __copy_from_user(&env->fpr[0], &fpu->si_float_regs[0],
-	                             (sizeof(unsigned long) * 32));
-#endif
+        err = copy_from_user(&env->fpr[0], fpu->si_float_regs[0],
+                             (sizeof(abi_ulong) * 32));
         err |= __get_user(env->fsr, &fpu->si_fsr);
 #if 0
         err |= __get_user(current->thread.fpqdepth, &fpu->si_fpqdepth);
Signature · 1 line
1.7.2.5
author
#3 lore raw

PATCH v2 3/6 qapi: avoid reserved keywords

In reply to Blue Swirl

Clang compiler complained about use of reserved word 'restrict' in SLIRP and QAPI.

Prefix C keywords with "q_", adjust SLIRP accordingly.

Signed-off-by Blue Swirl <[email protected]>
---
 net/slirp.c     |    6 +++---
 scripts/qapi.py |   16 ++++++++++++++++
 2 files changed, 19 insertions(+), 3 deletions(-)
net/slirp.c +3 -3
index 5c2e6b2..b818ea2 100644
@@ -722,9 +722,9 @@ int net_init_slirp(const NetClientOptions *opts, const char *name,
     net_init_slirp_configs(user->hostfwd, SLIRP_CFG_HOSTFWD);
     net_init_slirp_configs(user->guestfwd, 0);
 
-    ret = net_slirp_init(vlan, "user", name, user->restrict, vnet, user->host,
-                         user->hostname, user->tftp, user->bootfile,
-                         user->dhcpstart, user->dns, user->smb,
+    ret = net_slirp_init(vlan, "user", name, user->q_restrict, vnet,
+                         user->host, user->hostname, user->tftp,
+                         user->bootfile, user->dhcpstart, user->dns, user->smb,
                          user->smbserver);
 
     while (slirp_configs) {
scripts/qapi.py +16 -0
index 8082af3..80f28f8 100644
@@ -131,6 +131,22 @@ def camel_case(name):
     return new_name
 
 def c_var(name):
+    # ANSI X3J11/88-090, 3.1.1
+    c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue',
+                     'default', 'do', 'double', 'else', 'enum', 'extern', 'float',
+                     'for', 'goto', 'if', 'int', 'long', 'register', 'return',
+                     'short', 'signed', 'sizeof', 'static', 'struct', 'switch',
+                     'typedef', 'union', 'unsigned', 'void', 'volatile', 'while'])
+    # ISO/IEC 9899:1999, 6.4.1
+    c99_words = set(['inline', 'restrict', '_Bool', '_Complex', '_Imaginary'])
+    # ISO/IEC 9899:2011, 6.4.1
+    c11_words = set(['_Alignas', '_Alignof', '_Atomic', '_Generic', '_Noreturn',
+                     '_Static_assert', '_Thread_local'])
+    # GCC http://gcc.gnu.org/onlinedocs/gcc-4.7.1/gcc/C-Extensions.html
+    # excluding _.*
+    gcc_words = set(['asm', 'typeof'])
+    if name in c89_words | c99_words | c11_words | gcc_words:
+        return "q_" + name
     return name.replace('-', '_').lstrip("*")
 
 def c_fun(name):
Signature · 1 line
1.7.2.5
#5 lore raw

Re: [Qemu-devel] [PATCH v2 3/6] qapi: avoid reserved keywords

Quoted 10 lines · On Thu, 02 Aug 2012 10:32:59 +0200 Markus Armbruster <[email protected]> wrote:
Quoted 6 lines · Blue Swirl

Clang compiler complained about use of reserved word 'restrict' in SLIRP and QAPI.

Prefix C keywords with "q_", adjust SLIRP accordingly.

Signed-off-by Blue Swirl <[email protected]>

I like this solution.

Yeah, looks good.

Would be nice to add a note to docs/writing-qmp-commands.txt (can be in an incremental patch).

Quoted 3 lines
Reviewed-by Markus Armbruster <[email protected]>
author
#6 lore raw

PATCH v2 4/6 user: fix accidental AREG0 use

In reply to Blue Swirl

Global register AREG0 was always assumed to be usable in user-exec.c, but this is incorrect for several targets.

Fix with #ifdeffery and by using other variables.

Signed-off-by Blue Swirl <[email protected]>
---
 user-exec.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)
user-exec.c +12 -5
index 1a9c276..b9ea9dd 100644
@@ -18,7 +18,9 @@
  */
 #include "config.h"
 #include "cpu.h"
+#ifndef CONFIG_TCG_PASS_AREG0
 #include "dyngen-exec.h"
+#endif
 #include "disas.h"
 #include "tcg.h"
 

@@ -58,9 +60,11 @@ void cpu_resume_from_signal(CPUArchState *env1, void *puc)
     struct sigcontext *uc = puc;
 #endif
 
+#ifndef CONFIG_TCG_PASS_AREG0
     env = env1;
 
     /* XXX: restore cpu registers saved in host registers */
+#endif
 
     if (puc) {
         /* XXX: use siglongjmp ? */

@@ -74,8 +78,8 @@ void cpu_resume_from_signal(CPUArchState *env1, void *puc)
         sigprocmask(SIG_SETMASK, &uc->sc_mask, NULL);
 #endif
     }
-    env->exception_index = -1;
-    longjmp(env->jmp_env, 1);
+    env1->exception_index = -1;
+    longjmp(env1->jmp_env, 1);
 }
 
 /* 'pc' is the host PC at which the exception was raised. 'address' is

@@ -89,9 +93,11 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
     TranslationBlock *tb;
     int ret;
 
+#ifndef CONFIG_TCG_PASS_AREG0
     if (cpu_single_env) {
         env = cpu_single_env; /* XXX: find a correct solution for multithread */
     }
+#endif
 #if defined(DEBUG_SIGNAL)
     qemu_printf("qemu: SIGSEGV pc=0x%08lx address=%08lx w=%d oldset=0x%08lx\n",
                 pc, address, is_write, *(unsigned long *)old_set);

@@ -103,7 +109,8 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
     }
 
     /* see if it is an MMU fault */
-    ret = cpu_handle_mmu_fault(env, address, is_write, MMU_USER_IDX);
+    ret = cpu_handle_mmu_fault(cpu_single_env, address, is_write,
+                               MMU_USER_IDX);
     if (ret < 0) {
         return 0; /* not an MMU fault */
     }

@@ -115,13 +122,13 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
     if (tb) {
         /* the PC is inside the translated code. It means that we have
            a virtual CPU fault */
-        cpu_restore_state(tb, env, pc);
+        cpu_restore_state(tb, cpu_single_env, pc);
     }
 
     /* we restore the process signal mask as the sigreturn should
        do it (XXX: use sigsetjmp) */
     sigprocmask(SIG_SETMASK, old_set, NULL);
-    exception_action(env);
+    exception_action(cpu_single_env);
 
     /* never comes here */
     return 1;
Signature · 1 line
1.7.2.5
author
#7 lore raw

PATCH v2 5/6 xilinx_axienet: avoid useless self-assignment

In reply to Blue Swirl

Statement s=s; makes little sense, remove it. Spotted by Clang compiler.

Signed-off-by Blue Swirl <[email protected]>
---
 hw/xilinx_axienet.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)
hw/xilinx_axienet.c +0 -1
index e948505..e89a174 100644
@@ -648,7 +648,6 @@ static ssize_t eth_rx(VLANClientState *nc, const uint8_t *buf, size_t size)
     uint16_t csum16;
     int i;
 
-    s = s;
     DENET(qemu_log("%s: %zd bytes\n", __func__, size));
 
     unicast = ~buf[0] & 0x1;
Signature · 1 line
1.7.2.5
author
#8 lore raw

PATCH v2 6/6 scsi-bus: remove overlapping entry

In reply to Blue Swirl

LOAD_UNLOAD and START_STOP have same value, so the table entry is initialized twice. Spotted by Clang compiler.

Remove LOAD_UNLOAD entry since START_STOP entry already represents both.

Signed-off-by Blue Swirl <[email protected]>
---
 hw/scsi-bus.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)
hw/scsi-bus.c +0 -1
index e4ec19e..a8759bc 100644
@@ -1355,7 +1355,6 @@ static const char *scsi_command_name(uint8_t cmd)
         [ BLANK                    ] = "BLANK",
         [ MOVE_MEDIUM              ] = "MOVE_MEDIUM",
         [ EXCHANGE_MEDIUM          ] = "EXCHANGE MEDIUM",
-        [ LOAD_UNLOAD              ] = "LOAD_UNLOAD",
         [ READ_12                  ] = "READ_12",
         [ WRITE_12                 ] = "WRITE_12",
         [ ERASE_12                 ] = "ERASE_12/GET_PERFORMANCE",
Signature · 1 line
1.7.2.5
Reviewed-by on v2 6/6
#9 lore raw

Re: [Qemu-devel] [PATCH v2 6/6] scsi-bus: remove overlapping entry

[cc: SCSI maintainer]

Quoted 16 lines · Blue Swirl

LOAD_UNLOAD and START_STOP have same value, so the table entry is initialized twice. Spotted by Clang compiler.

Remove LOAD_UNLOAD entry since START_STOP entry already represents both.

Signed-off-by Blue Swirl <[email protected]>
---
 hw/scsi-bus.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)
hw/scsi-bus.c +0 -0
index e4ec19e..a8759bc 100644

hw/scsi-bus.c static const char *scsi_command_name(uint8_t cmd) · line 1360

Quoted 7 lines
hw/scsi-bus.c +0 -1
@@ -1355,7 +1355,6 @@ static const char *scsi_command_name(uint8_t cmd)
         [ BLANK                    ] = "BLANK",
         [ MOVE_MEDIUM              ] = "MOVE_MEDIUM",
         [ EXCHANGE_MEDIUM          ] = "EXCHANGE MEDIUM",
-        [ LOAD_UNLOAD              ] = "LOAD_UNLOAD",
         [ READ_12                  ] = "READ_12",
         [ WRITE_12                 ] = "WRITE_12",
         [ ERASE_12                 ] = "ERASE_12/GET_PERFORMANCE",

Missed in commit 15e58a21.

Reviewed-by Markus Armbruster <[email protected]>