Warning free build achieved, coreboot documentation updated

Warning free build achieved, coreboot documentation updated

[Pavel Roskin]

Hello!

I have fixed the remaining warnings in the coreboot build, so now GRUB
builds without warnings for all 7 supported platforms in the default
configuration.  Let's keep it this way.

I was unable to compile coreboot-v3, but I succeeded at compiling
coreboot-v2 for qemu.  It's not trivial, so I had to make a script.  The
script has been posted on http://grub.enbug.org/CoreBoot

-- 
Regards,
Pavel Roskin

Re: Warning free build achieved, coreboot documentation updated

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 2041 bytes --]

On Wed, Jun 17, 2009 at 11:47 PM, Pavel Roskin <proski@gnu.org> wrote:

> Hello!
>
> I have fixed the remaining warnings in the coreboot build, so now GRUB
> builds without warnings for all 7 supported platforms in the default
> configuration.  Let's keep it this way.
>
There are still warnings if you compile under FreeBSD. Attached patch fixes
all of them except one
The remaining is
util/hostdisk.c:1061: comparison between signed and unsigned
The problem comes from the following declaration
# define GRUB_LONG_MIN -2147483648UL
As you see we declare a negative number with UL. This works correctly on
i386 but what about other architectures? Can this warning be just silenced
with a cast or do we have a real problem here?
Index: util/hostdisk.c
===================================================================
--- util/hostdisk.c    (revision 2340)
+++ util/hostdisk.c    (working copy)
@@ -344,7 +344,7 @@
 #else /* ! __linux__ */
 #if defined (__FreeBSD__) || defined(__FreeBSD_kernel__)
   int sysctl_flags, sysctl_oldflags;
-  const size_t sysctl_size = sizeof (sysctl_flags);
+  size_t sysctl_size = sizeof (sysctl_flags);

   if (sysctlbyname ("kern.geom.debugflags", &sysctl_oldflags, &sysctl_size,
NULL, 0))
     {
@@ -833,6 +833,7 @@
 #endif
 }

+#if defined(__linux__) || defined(__CYGWIN__)
 static int
 device_is_wholedisk (const char *os_dev)
 {
@@ -842,6 +843,7 @@
     return 1;
   return 0;
 }
+#endif

 static int
 find_system_device (const char *os_dev)
@@ -1045,7 +1047,7 @@

     if (strncmp ("/dev/", os_dev, 5) == 0)
       {
-        char *p, *q;
+        const char *p, *q;
         long int n;

         for (p = os_dev + 5; *p; ++p)
@@ -1055,7 +1057,7 @@
               if (p)
                 {
                   p++;
-                  n = strtol (p, &q, 10);
+                  n = strtol (p, (char **) &q, 10);
                   if (p != q && n != GRUB_LONG_MIN && n != GRUB_LONG_MAX)
                     {
                       dos_part = (int) n - 1;


-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: Type: text/html, Size: 2589 bytes --]

Re: Warning free build achieved, coreboot documentation updated

[Isaac Dupree]

Vladimir 'phcoder' Serbinenko wrote:
> The remaining is
> util/hostdisk.c:1061: comparison between signed and unsigned
> The problem comes from the following declaration
> # define GRUB_LONG_MIN -2147483648UL
> As you see we declare a negative number with UL. This works correctly on
> i386 but what about other architectures? Can this warning be just silenced
> with a cast or do we have a real problem here?

it is portable since C standard defines unsigned numbers to have modular 
arithmetic.  However it therefore can't be silenced with a cast to a 
signed number (which doesn't have that guarantee!)  Note that casts 
between signed/unsigned of the same size are guaranteed to work as expected.

this should work without warnings I guess?:
# define GRUB_LONG_MIN (0UL - 2147483648UL)
do you want it to be of a signed type, though? then cast that to (long) 
afterwards.
(-1L - 2147483647L) would work too.

it's annoying isn't it that C doesn't actually have negative numeric 
literals :-)

-Isaac

Re: Warning free build achieved, coreboot documentation updated

[Pavel Roskin]

On Thu, 2009-06-18 at 02:25 +0200, Vladimir 'phcoder' Serbinenko wrote:
> Index: util/hostdisk.c
> ===================================================================
> --- util/hostdisk.c    (revision 2340)
> +++ util/hostdisk.c    (working copy)
> @@ -344,7 +344,7 @@
>  #else /* ! __linux__ */
>  #if defined (__FreeBSD__) || defined(__FreeBSD_kernel__)
>    int sysctl_flags, sysctl_oldflags;
> -  const size_t sysctl_size = sizeof (sysctl_flags);
> +  size_t sysctl_size = sizeof (sysctl_flags);
>  
>    if (sysctlbyname ("kern.geom.debugflags", &sysctl_oldflags,
> &sysctl_size, NULL, 0))

We use sysctl_size twice after that call.  If it's not constant, chances
are that it has changed.  Shouldn't we revert to the original value, or
should we use the returned value in the subsequent calls?

I've seen that warning and I considered that patch, but I didn't have a
chance to check the correctness of the change.  Just silencing the
warning is not good.  We should actually make sure that the problem is
fixed.

>      {
> @@ -833,6 +833,7 @@
>  #endif
>  }
>  
> +#if defined(__linux__) || defined(__CYGWIN__)
>  static int
>  device_is_wholedisk (const char *os_dev)
>  {
> @@ -842,6 +843,7 @@
>      return 1;
>    return 0;
>  }
> +#endif

That's good.
 
>  static int
>  find_system_device (const char *os_dev)
> @@ -1045,7 +1047,7 @@
>  
>      if (strncmp ("/dev/", os_dev, 5) == 0)
>        {
> -        char *p, *q;
> +        const char *p, *q;
>          long int n;
>  
>          for (p = os_dev + 5; *p; ++p)
> @@ -1055,7 +1057,7 @@
>                if (p)
>                  {
>                    p++;
> -                  n = strtol (p, &q, 10);
> +                  n = strtol (p, (char **) &q, 10);

Casts to remove "const" look like hacks to me.  Again, it's better to
keep a warning that to have such code.

-- 
Regards,
Pavel Roskin