[Qemu-devel] Re: [PATCH 1/5] Suppress some gcc warnings with -Wtype-limits

[Blue Swirl]

On Sun, Sep 5, 2010 at 9:26 AM, Michael S. Tsirkin <address@hidden> wrote:
> On Sun, Sep 05, 2010 at 09:06:10AM +0000, Blue Swirl wrote:
>> On Sun, Sep 5, 2010 at 7:54 AM, Michael S. Tsirkin <address@hidden> wrote:
>> > On Sat, Sep 04, 2010 at 05:21:24PM +0000, Blue Swirl wrote:
>> >> In the unsigned number space, the checks can be merged into one,
>> >> assuming that BLKDBG_EVEN_MAX is less than INT_MAX. Alternatively we
>> >> could have:
>> >>  -    if (event < 0 || event >= BLKDBG_EVENT_MAX) {
>> >>  +    if ((int)event < 0 || event >= BLKDBG_EVENT_MAX) {
>> >>
>> >> This would also implement the check that the writer of this code was
>> >> trying to make.
>> >> The important thing to note is however is that the check as it is now
>> >> is not correct.
>> >
>> > I agree. But it seems to indicate a bigger problem.
>> >
>> > If we are trying to pass in a negative value, which is not one
>> > of enum values, using BlkDebugEvent as type is just confusing,
>> > we should just pass int instead.
>>
>> AFAICT it's only possible to use the values listed in event_names in
>> blkdebug.c, other values are rejected. So the check should actually be
>> an assert() or it could even be removed.
>
> Sounds good.
>
>> >> >> How about adding assert(OMAP_EMIFS_BASE == 0) and commenting out the
>> >> >> check? Then if the value changes, the need to add the comparison back
>> >> >> will be obvious.
>> >> >
>> >> > This would work but it's weird.  The thing is it's currently a correct
>> >> > code and the check may be useless but it's the optimiser's task to
>> >> > remove it, not ours.  The compiler is not able to tell whether the
>> >> > check makes sense or nott, because the compiler only has access to
>> >> > preprocessed code.  So why should you let the compiler have anything
>> >> > to say on it.
>> >>
>> >> Good point. I'll try to invent something better.
>> >
>> > Use #pragma to supress the warning? Maybe we could wrap this in a macro ..
>>
>> Those lines may also desynch silently with changes to OMAP_EMIFS_BASE.
>>
>> I think the assertion is still the best way, it ensures that something
>> will happen if OMAP_EMIFS_BASE changes. We could for example remove
>> OMAP_EMIFS_BASE entirely (it's only used for the check), but someone
>> adding a new define could still forget to adjust the check anyway.
>
> We could replace it with a macro
> #define OMAP_EMIFS_VALID(addr) ((target_phys_addr_t)addr < OMAP_EMIFF_BASE)
> but all this does look artificial. And of course using type casts
> is always scary ...
>
> Would it help to have some inline functions that do the range checking 
> correctly?
> We have a couple of range helpers in pci.h, these could be moved out
> to range.h and we could add some more. As there act on u64 this will get
> the type limits mostly automatically right.

That seems to be the best solution, I get no warnings with this:

diff --git a/hw/omap1.c b/hw/omap1.c
index b00f870..8bf88e7 100644
--- a/hw/omap1.c
+++ b/hw/omap1.c
@@ -3672,14 +3672,25 @@ static int omap_validate_emiff_addr(struct
omap_mpu_state_s *s,
     return addr >= OMAP_EMIFF_BASE && addr < OMAP_EMIFF_BASE + s->sdram_size;
 }

+/* Get last byte of a range from offset + length.
+ * Undefined for ranges that wrap around 0. */
+static inline uint64_t range_get_last(uint64_t offset, uint64_t len)
+{
+    return offset + len - 1;
+}
+
+/* Check whether a given range covers a given byte. */
+static inline int range_covers_byte(uint64_t offset, uint64_t len,
+                                    uint64_t byte)
+{
+    return offset <= byte && byte <= range_get_last(offset, len);
+}
+
 static int omap_validate_emifs_addr(struct omap_mpu_state_s *s,
                 target_phys_addr_t addr)
 {
-    /* If OMAP_EMIFS_BASE ever becomes nonzero, adjust the check below
-       to also include the lower bound check like
-       addr >= OMAP_EMIFS_BASE && addr < OMAP_EMIFF_BASE */
-    assert(OMAP_EMIFS_BASE == 0);
-    return addr < OMAP_EMIFF_BASE;
+    return range_covers_byte(OMAP_EMIFS_BASE,
+                             OMAP_EMIFF_BASE - OMAP_EMIFS_BASE, addr);
 }

 static int omap_validate_imif_addr(struct omap_mpu_state_s *s,

I'll add range.h and respin the patches.