Patchwork YABEL compilation warnings

login
register
about
Submitter Myles Watson
Date 2010-03-05 15:13:39
Message ID <2831fecf1003050713h77bd7abbx245a8bd6fa96a595@mail.gmail.com>
Download mbox | patch
Permalink /patch/1012/
State Accepted
Headers show

Comments

Myles Watson - 2010-03-05 15:13:39
On Fri, Mar 5, 2010 at 8:04 AM, Myles Watson <mylesgw@gmail.com> wrote:

> This patch removes most of the rest of the compilation warnings for me.
>
> 1. Move run_bios prototype to device.h
> 2. Use time.h for get_time()
>
2b. Move tb_freq into functions.c instead of the time.h

> 3. Move read_io and write_io to io.c and make them static
> 4. Make a couple of functions static in interrupt.c
> 5. Refactor a cast from char[] to u64 to get rid of potential alignment
> problems and a warning
>
> The only ones left are "unused function" warnings.
>
> I think we should get rid of that warning, since we conditionally call
> functions based on debugging and various config variables.  Is there a case
> where it helps enough to justify all the warnings?
>
> This next part isn't part of the patch, but applying it makes qemu compile
> with yabel (with and without debugging).
>
> Index: Makefile
> ===================================================================
> --- Makefile    (revision 5186)
> +++ Makefile    (working copy)
> @@ -239,7 +239,7 @@
>  CFLAGS = $(INCLUDES) -Os -nostdinc
>  CFLAGS += -nostdlib -Wall -Wundef -Wstrict-prototypes -Wmissing-prototypes
>  CFLAGS += -Wwrite-strings -Wredundant-decls -Wno-trigraphs
> -CFLAGS += -Wstrict-aliasing -Wshadow
>

I meant no-unused-function:

> +CFLAGS += -Wstrict-aliasing -Wshadow -Wno-unused-function
>


>  ifeq ($(CONFIG_WARNINGS_ARE_ERRORS),y)
>  CFLAGS += -Werror
>  endif
>

 Signed-off-by: Myles Watson <mylesgw@gmail.com>

Thanks,
Myles
Stefan Reinauer - 2010-03-05 17:28:15
On 3/5/10 4:13 PM, Myles Watson wrote:
>
>     I think we should get rid of that warning, since we conditionally
>     call functions based on debugging and various config variables.  
>
I think we should instead just drop CONFIG_WARNINGS_ARE_ERRORS for Qemu
rather than dropping warnings. Or maybe only add -Wno-unused-function if
CONFIG_WARNINGS_ARE_ERRORS is active, at least.  Or maybe add an Option
"Warnings good for Developers"?

>     Is there a case where it helps enough to justify all the warnings?
>

Yes, they indicate that there is dead code. This is, not only but
especially useful to recognize if / how code should / could be restructured.

>  Signed-off-by: Myles Watson <mylesgw@gmail.com
> <mailto:mylesgw@gmail.com>>
With the -Wno-unused-functions taken care of:

Acked-by: Stefan Reinauer <stepan@coresystems.de>
Myles Watson - 2010-03-05 18:28:15
On Fri, Mar 5, 2010 at 10:28 AM, Stefan Reinauer <stepan@coresystems.de>wrote:

>  On 3/5/10 4:13 PM, Myles Watson wrote:
>
>  I think we should get rid of that warning, since we conditionally call
>> functions based on debugging and various config variables.
>
>  I think we should instead just drop CONFIG_WARNINGS_ARE_ERRORS for Qemu
> rather than dropping warnings.
>
I think that setting for Qemu has reduced the number of warnings in our code
by a lot.  It's easier for the original developer to deal with warnings when
the code is written.


> Or maybe only add -Wno-unused-function if
>
CONFIG_WARNINGS_ARE_ERRORS is active, at least.  Or maybe add an Option
> "Warnings good for Developers"?
>
I can see your point.  I'd rather not have to special case warnings either.
What would you suggest for the two emulator functions in yabel that are
unused right now?  Should we comment them out until they are used?


>  Is there a case where it helps enough to justify all the warnings?
>>
>   Yes, they indicate that there is dead code. This is, not only but
> especially useful to recognize if / how code should / could be restructured.
>
>
I worry that having too many warnings makes it so that "important" ones
(ones that cause bugs) get missed.

>   Signed-off-by: Myles Watson <mylesgw@gmail.com>
>
> With the -Wno-unused-functions taken care of:
>

I wasn't sure how you wanted it taken care of.  I left it for another patch.


> Acked-by: Stefan Reinauer <stepan@coresystems.de> <stepan@coresystems.de>
>

Rev 5191.

Thanks,
Myles

Patch

Index: svn/src/devices/pci_device.c
===================================================================
--- svn.orig/src/devices/pci_device.c
+++ svn/src/devices/pci_device.c
@@ -652,7 +652,6 @@  void pci_dev_set_subsystem(struct device
 void pci_dev_init(struct device *dev)
 {
 #if CONFIG_PCI_ROM_RUN == 1 || CONFIG_VGA_ROM_RUN == 1
-	void run_bios(struct device *dev, unsigned long addr);
 	struct rom_header *rom, *ram;
 
 	if (CONFIG_PCI_ROM_RUN != 1 && /* Only execute VGA ROMs. */
Index: svn/src/include/device/device.h
===================================================================
--- svn.orig/src/include/device/device.h
+++ svn/src/include/device/device.h
@@ -117,6 +117,9 @@  const char *bus_path(struct bus *bus);
 void dev_set_enabled(device_t dev, int enable);
 void disable_children(struct bus *bus);
 
+/* Option ROM helper functions */
+void run_bios(struct device *dev, unsigned long addr);
+
 /* Helper functions */
 device_t find_dev_path(struct bus *parent, struct device_path *path);
 device_t alloc_find_dev(struct bus *parent, struct device_path *path);
Index: svn/util/x86emu/yabel/compat/functions.c
===================================================================
--- svn.orig/util/x86emu/yabel/compat/functions.c
+++ svn/util/x86emu/yabel/compat/functions.c
@@ -18,6 +18,7 @@ 
 #include <device/device.h>
 #include "../debug.h"
 #include "../biosemu.h"
+#include "../compat/time.h"
 
 #define VMEM_SIZE (1024 * 1024) /* 1 MB */
 
@@ -52,6 +53,8 @@  void run_bios(struct device * dev, unsig
 	}
 }
 
+unsigned long tb_freq = 0;
+
 u64 get_time(void)
 {
     u64 act;
@@ -64,50 +67,3 @@  u64 get_time(void)
     act = ((u64) edx << 32) | eax; 
     return act;
 }
-
-unsigned int
-read_io(void *addr, size_t sz)
-{
-        unsigned int ret;
-	/* since we are using inb instructions, we need the port number as 16bit value */
-	u16 port = (u16)(u32) addr;
-
-        switch (sz) {
-        case 1:
-		asm volatile ("inb %1, %b0" : "=a"(ret) : "d" (port));
-                break;
-        case 2:
-		asm volatile ("inw %1, %w0" : "=a"(ret) : "d" (port));
-                break;
-        case 4:
-		asm volatile ("inl %1, %0" : "=a"(ret) : "d" (port));
-                break;
-        default:
-                ret = 0;
-        }
-
-        return ret;
-}
-
-int
-write_io(void *addr, unsigned int value, size_t sz)
-{
-	u16 port = (u16)(u32) addr;
-        switch (sz) {
-	/* since we are using inb instructions, we need the port number as 16bit value */
-        case 1:
-		asm volatile ("outb %b0, %1" : : "a"(value), "d" (port));
-                break;
-        case 2:
-		asm volatile ("outw %w0, %1" : : "a"(value), "d" (port));
-                break;
-        case 4:
-		asm volatile ("outl %0, %1" : : "a"(value), "d" (port));
-                break;
-        default:
-                return -1;
-        }
-
-        return 0;
-}
-
Index: svn/util/x86emu/yabel/compat/time.h
===================================================================
--- svn.orig/util/x86emu/yabel/compat/time.h
+++ svn/util/x86emu/yabel/compat/time.h
@@ -13,5 +13,6 @@ 
 #define _BIOSEMU_COMPAT_TIME_H
 
 /* TODO: check how this works in x86 */
-static unsigned long tb_freq = 0;
+extern unsigned long tb_freq;
+u64 get_time(void);
 #endif 
Index: svn/util/x86emu/yabel/io.c
===================================================================
--- svn.orig/util/x86emu/yabel/io.c
+++ svn/util/x86emu/yabel/io.c
@@ -24,12 +24,51 @@ 
 #include <device/pci_ops.h>
 #endif
 
-// those are defined in net-snk/oflib/pci.c
-extern unsigned int read_io(void *, size_t);
-extern int write_io(void *, unsigned int, size_t);
+static unsigned int
+read_io(void *addr, size_t sz)
+{
+        unsigned int ret;
+	/* since we are using inb instructions, we need the port number as 16bit value */
+	u16 port = (u16)(u32) addr;
+
+        switch (sz) {
+        case 1:
+		asm volatile ("inb %1, %b0" : "=a"(ret) : "d" (port));
+                break;
+        case 2:
+		asm volatile ("inw %1, %w0" : "=a"(ret) : "d" (port));
+                break;
+        case 4:
+		asm volatile ("inl %1, %0" : "=a"(ret) : "d" (port));
+                break;
+        default:
+                ret = 0;
+        }
+
+        return ret;
+}
+
+static int
+write_io(void *addr, unsigned int value, size_t sz)
+{
+	u16 port = (u16)(u32) addr;
+        switch (sz) {
+	/* since we are using inb instructions, we need the port number as 16bit value */
+        case 1:
+		asm volatile ("outb %b0, %1" : : "a"(value), "d" (port));
+                break;
+        case 2:
+		asm volatile ("outw %w0, %1" : : "a"(value), "d" (port));
+                break;
+        case 4:
+		asm volatile ("outl %0, %1" : : "a"(value), "d" (port));
+                break;
+        default:
+                return -1;
+        }
 
-//defined in net-snk/kernel/timer.c
-extern u64 get_time(void);
+        return 0;
+}
 
 #ifdef CONFIG_ARCH_X86
 #include <arch/io.h>
Index: svn/util/x86emu/yabel/mem.c
===================================================================
--- svn.orig/util/x86emu/yabel/mem.c
+++ svn/util/x86emu/yabel/mem.c
@@ -159,9 +159,6 @@  u32 ebda_size;
 #define DEBUG_CHECK_VMEM_WRITE(_addr, _val)
 #endif
 
-//defined in net-snk/kernel/timer.c
-extern u64 get_time(void);
-
 void update_time(u32);
 
 #if !defined(CONFIG_YABEL_DIRECTHW) || (!CONFIG_YABEL_DIRECTHW)
Index: svn/util/x86emu/yabel/interrupt.c
===================================================================
--- svn.orig/util/x86emu/yabel/interrupt.c
+++ svn/util/x86emu/yabel/interrupt.c
@@ -31,7 +31,7 @@ 
 
 
 //setup to run the code at the address, that the Interrupt Vector points to...
-void
+static void
 setupInt(int intNum)
 {
 	DEBUG_PRINTF_INTR("%s(%x): executing interrupt handler @%08x\n",
@@ -50,7 +50,7 @@  setupInt(int intNum)
 }
 
 // handle int10 (VGA BIOS Interrupt)
-void
+static void
 handleInt10(void)
 {
 	// the data for INT10 is stored in BDA (0000:0400h) offset 49h-66h
@@ -207,7 +207,7 @@  static u8 keycode_table[256] = {
 
 ;
 
-void
+static void
 translate_keycode(u64 * keycode)
 {
 	u8 scan_code = 0;
@@ -233,7 +233,7 @@  translate_keycode(u64 * keycode)
 }
 
 // handle int16 (Keyboard BIOS Interrupt)
-void
+static void
 handleInt16(void)
 {
 	// keyboard buffer is in BIOS Memory Area:
@@ -319,7 +319,7 @@  handleInt16(void)
 }
 
 // handle int1a (PCI BIOS Interrupt)
-void
+static void
 handleInt1a(void)
 {
 	// function number in AX
Index: svn/util/x86emu/yabel/vbe.c
===================================================================
--- svn.orig/util/x86emu/yabel/vbe.c
+++ svn/util/x86emu/yabel/vbe.c
@@ -570,8 +570,17 @@  vbe_get_info(void)
 		     sizeof(ddc_info.edid_block_zero));
 	}
 #endif
-	if (*((u64 *) ddc_info.edid_block_zero) !=
-	    (u64) 0x00FFFFFFFFFFFF00ULL) {
+/* This could fail because of alignment issues, so use a longer form.
+	*((u64 *) ddc_info.edid_block_zero) != (u64) 0x00FFFFFFFFFFFF00ULL
+*/
+	if (ddc_info.edid_block_zero[0] != 0x00 ||
+	    ddc_info.edid_block_zero[1] != 0xFF ||
+	    ddc_info.edid_block_zero[2] != 0xFF ||
+	    ddc_info.edid_block_zero[3] != 0xFF ||
+	    ddc_info.edid_block_zero[4] != 0xFF ||
+	    ddc_info.edid_block_zero[5] != 0xFF ||
+	    ddc_info.edid_block_zero[6] != 0xFF ||
+	    ddc_info.edid_block_zero[7] != 0x00 ) {
 		// invalid EDID signature... probably no monitor
 
 		output->display_type = 0x0;