Fix build errors and warnings

Several build errors and warnings have been fixed, including a
recent breakage of DEBUG=1 builds. Several bit fields in CSAL
structures are now explicitly 'unsigned'; previously their
signedness would vary depending on platform. This is not expected
to cause any portability issues. The default compiler options
(in build/makefile) now include -Wpedantic -Wextra .

We are investigating how to improve testing coverage in future.

build/makefile

@@ -34,7 +34,7 @@ include ../makefile-arch.inc
 
 # default settings - allow for cross compile of library 
 CC=$(CROSS_COMPILE)gcc
-CFLAGS=-Wall -Wno-switch -Werror
+CFLAGS=-Wall -Wpedantic -Wextra -Wno-switch -Werror
 
 LIB_DIR = ../lib/$(ARCH)/rel
 LIB_DIR_BM=../lib/$(ARCH)/rel_bm

include/cs_memap.h

@@ -60,7 +60,7 @@ uint32_t cs_memap_read32(cs_device_t device, cs_physaddr_t addr);
  * \param addr    the physical address, in MEM-AP space, to write to
  * \param data    the 32-bit data to be written
  */
-void cs_memap_write32(cs_device_t device, cs_physaddr_t addr, uint32_t data);
+int cs_memap_write32(cs_device_t device, cs_physaddr_t addr, uint32_t data);
 
 /**
  * Read a 64-bit value from a location in a MEM-AP's target address space
@@ -79,7 +79,7 @@ uint64_t cs_memap_read64(cs_device_t device, cs_physaddr_t addr);
  * \param addr    the physical address, in MEM-AP space, to write to
  * \param data    the 64-bit data to be written
  */
-void cs_memap_write64(cs_device_t device, cs_physaddr_t addr, uint64_t data);
+int cs_memap_write64(cs_device_t device, cs_physaddr_t addr, uint64_t data);
 
 
 /**
@@ -98,7 +98,7 @@ int cs_memap_check_error(cs_device_t device, int reset);
  * \param device  the MEM-AP device
  * \param addr    the address in the MEM-AP's address space
  */
-void cs_memap_write_TAR(cs_device_t device, cs_physaddr_t addr);
+int cs_memap_write_TAR(cs_device_t device, cs_physaddr_t addr);
 
 /**
  * Read the MEM-AP's Transfer Address Register directly.

include/cs_types.h

@@ -139,6 +139,7 @@ typedef int cs_atid_t;
  * to be some hierarchical nesting of power domains.
  */
 typedef unsigned int cs_power_domain_t;
+#define CS_UNKNOWN_POWER_DOMAIN (cs_power_domain_t)(-1)
 
 /** \brief Device Type
  * 

source/cs_access_cmnfns.c

@@ -607,17 +607,16 @@ int _cs_is_lockable(struct cs_device *d)
 int _cs_unlock(struct cs_device *d)
 {
     if (!d->is_unlocked) {
-	_cs_write_wo_traced(d, CS_LAR, CS_KEY, "LAR");
-	d->is_unlocked = 1;
+        _cs_write_wo_traced(d, CS_LAR, CS_KEY, "LAR");
+        d->is_unlocked = 1;
     }
     if (DCHECK(d)) {
-	uint32_t lsr = _cs_read(d, CS_LSR);
-	if ((lsr & 3) == 3) {
-	    /* Implemented (bit 0) and still locked (bit 1) */
-	    diagf("!%" CS_PHYSFMT ": after unlock, LSR=%08X\n",
-		  d->phys_addr, lsr);
+        uint32_t lsr = _cs_read(d, CS_LSR);
+        if ((lsr & 3) == 3) {
+            /* Implemented (bit 0) and still locked (bit 1) */
+            diagf("!%" CS_PHYSFMT ": after unlock, LSR=%08" PRIX32 "\n", d->phys_addr, lsr);
             return -1;
-	}
+        }
     }
     return 0;
 }
@@ -633,7 +632,7 @@ int _cs_lock(struct cs_device *d)
 	unsigned int lsr = _cs_read(d, CS_LSR);
 	if ((lsr & 3) == 1) {
 	    /* Implemented (bit 0) but not locked (bit 1) */
-	    diagf("!%" CS_PHYSFMT ": after lock, LSR=%08X\n",
+	    diagf("!%" CS_PHYSFMT ": after lock, LSR=%08" PRIX32 "\n",
 		  d->phys_addr, lsr);
             return -1;
 	}

source/cs_access_cmnfns.h

@@ -129,8 +129,8 @@ struct cs_device {
     unsigned int devaff0;	  /**< Device affinity register for CPU-affine devices - might be CPU's MPIDR, but might also be zero */
     cs_cpu_t affine_cpu;	  /**< Set by the user via the API */
     cs_power_domain_t power_domain;
-    int is_unlocked:1;
-    int is_permanently_unlocked:1;
+    unsigned int is_unlocked:1;   /**< Device is (as far as we know) unlocked, either permanently or via LAR */
+    unsigned int is_permanently_unlocked:1;   /**< LAR not implemented, device does not need to be unlocked */
 
 #if DIAG
     cs_diag_level_t diag_tracing;     /**< Diagnostic messages for actions on this device */
@@ -186,9 +186,9 @@ struct cs_device {
         } etm;
         struct etb_props {
             unsigned int buffer_size_bytes;
-            int currently_reading:1;
-            int finished_reading:1;
-            int is_tmc_device:1;	/* This is a TMC, as opposed to e.g. a classic ETB */
+            unsigned int currently_reading:1;
+            unsigned int finished_reading:1;
+            unsigned int is_tmc_device:1;     /**< This is a TMC, as opposed to e.g. a classic ETB */
             /* For pre-TMC ETBs, the read and write pointers address words within
                the buffer RAM - for CoreSight ETBs the buffer is always 32-bit RAM,
                so the pointers are scaled by 4.  Only very old pre-CoreSight ETBs
@@ -209,16 +209,16 @@ struct cs_device {
             unsigned int n_masters;
             unsigned int current_master;
             unsigned char **ext_ports;	  /**< array of pointers to mappings of master ports. */
-            int basic_ports:1;
+            unsigned int basic_ports:1;
             stm_static_config_t s_config;     /**< RO features registers */
         } stm;
         struct ts_gen_props {
             cs_ts_gen_config_t config;
         } ts;
         struct memap_props {
-            int DAR_present:1;            /**< Direct Access Registers are available */
-            int TAR_valid:1;              /**< We have a cached copy of the TAR */
-            int memap_LPAE:1;             /**< Large Physical Addresses implemented */
+            unsigned int DAR_present:1;   /**< Direct Access Registers are available */
+            unsigned int TAR_valid:1;     /**< We have a cached copy of the TAR */
+            unsigned int memap_LPAE:1;    /**< Large Physical Addresses implemented */
             unsigned long cached_TAR;     /**< Cached copy of the TAR */
         } memap;
         struct ela_props {
@@ -261,13 +261,13 @@ struct cs_global {
 #ifdef DIAG
     FILE *diag_fd;                 /**< Output stream for diagnostics */
 #endif
-    int init_called:1;             /**< cs_init() has been called */
-    int registration_open:1;       /**< cs_registration_complete() not yet called */
-    int force_writes:1;            /**< Always write back on RMW operations even when unchanged */
-    int diag_checking:1;	   /**< Default diag setting for new devices */
-    int phys_addr_lpae:1;	   /**< 1 if built with LPAE */
-    int virt_addr_64bit:1;         /**< 1 if built with 64 bit virtual addresses */
-    int devaff0_used:1;            /**< Non-zero DEVAFF0 has been seen */
+    unsigned int init_called:1;           /**< cs_init() has been called */
+    unsigned int registration_open:1;     /**< cs_registration_complete() not yet called */
+    unsigned int force_writes:1;          /**< Always write back on RMW operations even when unchanged */
+    unsigned int diag_checking:1;         /**< Default diag setting for new devices */
+    unsigned int phys_addr_lpae:1;        /**< 1 if built with LPAE */
+    unsigned int virt_addr_64bit:1;       /**< 1 if built with 64 bit virtual addresses */
+    unsigned int devaff0_used:1;          /**< Non-zero DEVAFF0 has been seen */
     cs_diag_level_t diag_tracing_default;    /**< Default trace setting for new devices */
     unsigned int n_api_errors;
     unsigned int n_devices;
@@ -282,7 +282,7 @@ struct cs_global {
  */
 static inline struct cs_device *cs_get_device_struct(cs_device_t dev)
 {
-    assert(dev != ERRDESC);
+    assert(dev != CS_ERRDESC);
     return (struct cs_device *)(dev);
 }
 
@@ -293,11 +293,6 @@ static inline struct cs_device *cs_get_device_struct(cs_device_t dev)
  */
 #define DEVDESC(d) ((void *)(d))
 
-/**
- * The special opaque device descriptor indicating an error
- */
-#define ERRDESC ((void *)0)
-
 
 /**
  * Diagnostics are provided to aid in debugging CSAL.
@@ -323,7 +318,7 @@ static inline struct cs_device *cs_get_device_struct(cs_device_t dev)
 #if CHECK
 #define DCHECK(d) (d->glob->diag_checking)
 #else				/* !CHECK */
-#define DCHECK(d) 0
+#define DCHECK(d) (0)
 #endif				/* CHECK */
 
 

source/cs_etm.c

@@ -49,6 +49,7 @@ int _cs_etm_static_config_init(struct cs_device *d)
     return 0;
 }
 
+
 /*
   Return the ETM version in encoded form.  Compare it against one of
   the provided constants.
@@ -59,6 +60,7 @@ unsigned int _cs_etm_version(struct cs_device *d)
     return (d->v.etm.etmidr >> 4) & 0xFF;
 }
 
+
 int _cs_etm_enable_programming(struct cs_device *d)
 {
     int rc;
@@ -287,7 +289,7 @@ static void _cs_etm_config_clean(struct cs_etm_config *c)
         /* config init will have zeroed out the values, but we
            do need to change the access type to 001 (Execute),
            as not all ETMs support access type 000 (Fetch) */
-        for (i = 0; i < c->sc->ccr.s.n_addr_comp_pairs * 2; ++i) {
+        for (i = 0; i < c->sc->ccr.s.n_addr_comp_pairs * 2U; ++i) {
             c->addr_comp[i].access_type |= 1;
         }
     }
@@ -416,7 +418,7 @@ int cs_etm_config_get(cs_device_t dev, struct cs_etm_config *c)
 
     if (c->flags & CS_ETMC_ADDR_COMP) {
         //    for (i = 0; i < c->sc.s.n_addr_comp_pairs * 2; ++i) {
-        for (i = 0; i < c->sc->ccr.s.n_addr_comp_pairs * 2; ++i) {
+        for (i = 0; i < c->sc->ccr.s.n_addr_comp_pairs * 2U; ++i) {
             if (c->addr_comp_mask & (1U << i)) {
                 c->addr_comp[i].address = _cs_read(d, CS_ETMACVR(i));
                 c->addr_comp[i].access_type = _cs_read(d, CS_ETMACTR(i));
@@ -426,7 +428,7 @@ int cs_etm_config_get(cs_device_t dev, struct cs_etm_config *c)
 
     c->data_comp_mask &= onebits(c->sc->ccr.s.n_data_comp);
     if ((c->flags & CS_ETMC_DATA_COMP) && (!is_ptm)) {
-        //    for (i = 0; i < c->sc.s.n_addr_comp_pairs * 2; ++i) {
+        //    for (i = 0; i < c->sc.s.n_addr_comp_pairs * 2U; ++i) {
         for (i = 0; i < c->sc->ccr.s.n_data_comp; ++i) {
             if (c->data_comp_mask & (1U << i)) {
                 c->data_comp[i].value = _cs_read(d, CS_ETMDCVR(i));
@@ -529,7 +531,7 @@ int cs_etm_config_put(cs_device_t dev, struct cs_etm_config *c)
         _cs_write(d, CS_ETMTSEVR, c->timestamp_event);
     }
     if (c->flags & CS_ETMC_ADDR_COMP) {
-        for (i = 0; i < c->sc->ccr.s.n_addr_comp_pairs * 2; ++i) {
+        for (i = 0; i < c->sc->ccr.s.n_addr_comp_pairs * 2U; ++i) {
             if (c->addr_comp_mask & (1U << i)) {
                 _cs_write(d, CS_ETMACVR(i), c->addr_comp[i].address);
                 atype = (c->addr_comp[i].access_type & 7);
@@ -547,7 +549,7 @@ int cs_etm_config_put(cs_device_t dev, struct cs_etm_config *c)
     }
     c->data_comp_mask &= onebits(c->sc->ccr.s.n_data_comp);
     if ((c->flags & CS_ETMC_DATA_COMP) && has_data_trace) {
-        //    for (i = 0; i < c->sc.s.n_addr_comp_pairs * 2; ++i) {
+        //    for (i = 0; i < c->sc.s.n_addr_comp_pairs * 2U; ++i) {
         for (i = 0; i < c->sc->ccr.s.n_data_comp; ++i) {
             if (c->data_comp_mask & (1U << i)) {
                 _cs_write(d, CS_ETMDCVR(i), c->data_comp[i].value);
@@ -834,7 +836,7 @@ int cs_etm_config_print(struct cs_etm_config *c)
     if (c->flags & CS_ETMC_ADDR_COMP) {
         printf("  Address comparators: %u\n",
                c->sc->ccr.s.n_addr_comp_pairs * 2);
-        for (i = 0; i < c->sc->ccr.s.n_addr_comp_pairs * 2; ++i) {
+        for (i = 0; i < c->sc->ccr.s.n_addr_comp_pairs * 2U; ++i) {
             if (c->addr_comp_mask & (1U << i)) {
                 static char const *const tnames[8] = {
                     "fetch", "execute", "ex-pass", "ex-fail",

source/cs_etm_v4.c

@@ -831,6 +831,9 @@ int _cs_etm_v4_config_print(struct cs_device *d, cs_etmv4_config_t * c)
         "All QE enabled"
     };
 
+#ifndef DEBUG
+    (void)d;    /* The device parameter is unused outside of the asserts */
+#endif
     assert(d->type == DEV_ETM);
     assert(CS_ETMVERSION_MAJOR(_cs_etm_version(d)) >= CS_ETMVERSION_ETMv4);
     if (c->flags == 0) {

source/cs_map_local.c

@@ -27,9 +27,12 @@
 #include <sys/mman.h>
 #endif /* UNIX_USERSPACE */
 
+#define UNUSED_PARAMETER(x) ((void)(x))
+
 
 /*
-  Map a region (generally 4K) of physical memory.
+  Map a region (generally 4K) of physical memory, returning an address
+  usable by the caller - a virtual address in the case of non-baremetal.
   Return NULL if unsuccessful.
 */
 void *io_map(cs_physaddr_t addr, unsigned int size, int writable)
@@ -60,8 +63,15 @@ void *io_map(cs_physaddr_t addr, unsigned int size, int writable)
     localv = (unsigned char *)0xBAD;
 #endif
 #elif defined(UNIX_KERNEL)
+    UNUSED_PARAMETER(writable);
     localv = ioremap(addr, size);
 #else
+    /* Bare-metal: the caller directly accesses the physical memory.
+       Note that the combination of BAREMETAL and LPAE is not supported
+       with a 32-bit target, and will likely error here when trying to
+       cast a 64-bit cs_physaddr_t to a pointer. */
+    UNUSED_PARAMETER(writable);
+    UNUSED_PARAMETER(size);
     localv = (void *)addr;
 #endif
     return localv;
@@ -82,8 +92,11 @@ void io_unmap(void volatile *addr, unsigned int size)
     (void)munmap((void *)addr, size);
 #endif
 #elif defined(UNIX_KERNEL)
+    UNUSED_PARAMETER(size);
     iounmap(addr);
 #else
+    UNUSED_PARAMETER(addr);
+    UNUSED_PARAMETER(size);
     /* do nothing */
 #endif
 }

source/cs_memap.c

@@ -79,18 +79,18 @@ uint64_t cs_memap_read64(cs_device_t dev, cs_physaddr_t addr)
 /*
  * Write data to a memory location, via a MEM-AP.
  */
-void cs_memap_write32(cs_device_t dev, cs_physaddr_t addr, uint32_t data)
+int cs_memap_write32(cs_device_t dev, cs_physaddr_t addr, uint32_t data)
 {
     struct cs_device *d = DEV(dev);
     unsigned int reg = cs_memap_prepare(d, addr);
-    _cs_write(d, reg, data);
+    return _cs_write(d, reg, data);
 }
 
 
-void cs_memap_write64(cs_device_t dev, cs_physaddr_t addr, uint64_t data)
+int cs_memap_write64(cs_device_t dev, cs_physaddr_t addr, uint64_t data)
 {
     cs_memap_write32(dev, addr, (uint32_t)data);
-    cs_memap_write32(dev, addr+4, (uint32_t)(data >> 32));
+    return cs_memap_write32(dev, addr+4, (uint32_t)(data >> 32));
 }
 
 
@@ -113,17 +113,22 @@ cs_physaddr_t cs_memap_read_TAR(cs_device_t dev)
 /*
  * Set the current value of the TAR
  */
-void cs_memap_write_TAR(cs_device_t dev, cs_physaddr_t addr)
+int cs_memap_write_TAR(cs_device_t dev, cs_physaddr_t addr)
 {
+    int rc;
     struct cs_device *d = DEV(dev);
-    _cs_write(d, CS_MEMAP_TAR, addr);
+    rc = _cs_write(d, CS_MEMAP_TAR, addr);
+    if (rc) {
+        return rc;
+    }
 #ifdef LPAE
     if (d->v.memap.memap_LPAE) {
         _cs_write(d, CS_MEMAP_TARHI, (addr >> 32));
     }
 #endif
     d->v.memap.cached_TAR = addr;
     d->v.memap.TAR_valid = 1;
+    return 0;
 }
 
 

source/cs_reg_access.c

@@ -132,6 +132,7 @@ int cs_device_diag_set(cs_device_t dev, int tracing)
 
 void cs_device_data_barrier(cs_device_t dev)
 {
+    (void)dev;    /* Currently none of the barrier methods are device-specific */
 #ifndef USE_DEVMEMD
 #if __ARM_ARCH >= 7
     __asm__ __volatile__("dmb sy");
@@ -143,6 +144,7 @@ void cs_device_data_barrier(cs_device_t dev)
 
 void cs_device_instruction_barrier(cs_device_t dev)
 {
+    (void)dev;    /* Currently none of the barrier methods are device-specific */
 #ifndef USE_DEVMEMD
 #if __ARM_ARCH >= 7
     __asm__ __volatile__("dsb sy");

source/cs_sw_stim.c

@@ -121,7 +121,7 @@ int cs_trace_stimulus(cs_device_t dev, unsigned int port, uint32_t value)
 
     assert(cs_device_has_class
            (dev, CS_DEVCLASS_SOURCE | CS_DEVCLASS_SWSTIM));
-    assert(port < cs_trace_swstim_get_port_count(dev));
+    assert((int)port < cs_trace_swstim_get_port_count(dev));
 
     if (d->type == DEV_ITM) {
         /* "The lock access mechanism is not present for any access to stimulus
@@ -294,7 +294,7 @@ int cs_stm_ext_write(cs_device_t dev, const unsigned int port,
     assert(d->type == DEV_STM);
     assert(STM_OP_VALID(trans_type));
     assert((value != 0) || !STM_OP_DATA(trans_type));
-    assert(port < cs_trace_swstim_get_port_count(dev));
+    assert((int)port < cs_trace_swstim_get_port_count(dev));
     assert(master != NULL);
     assert((value == 0) || (length > 0));