On Thu, Aug 07, 2008 at 12:21:05PM +0200, svn@coreboot.org wrote:
Author: stepan Date: 2008-08-07 12:21:05 +0200 (Thu, 07 Aug 2008) New Revision: 3474
Added: trunk/payloads/libpayload/drivers/options.c Modified: trunk/payloads/libpayload/drivers/Makefile.inc trunk/payloads/libpayload/i386/coreboot.c trunk/payloads/libpayload/include/coreboot_tables.h trunk/payloads/libpayload/include/sysinfo.h Log: add get_option to libpayload, so coreboot cmos options can be queried.
Signed-off-by: Stefan Reinauer stepan@coresystems.de Acked-by: Jordan Crouse jordan.crouse@amd.com Acked-by: Peter Stuge peter@stuge.se
Sorry.
This breaks my build. I should have tested. :\
cc -m32 -Wall -Werror -fno-stack-protector -nostdinc -I./include -I/usr/lib/gcc/i686-pc-linux-gnu/4.2.2/include -c -o drivers/options.o drivers/options.c cc1: warnings being treated as errors drivers/options.c: In function 'get_option': drivers/options.c:93: warning: pointer targets in passing argument 1 of 'memcmp' differ in signedness make: *** [drivers/options.o] Error 1
From the patch:
if (memcmp(cmos_entry->name, name, len))
..
+#define CB_TAG_OPTION 0x00c9 +#define CMOS_MAX_NAME_LENGTH 32 +struct cb_cmos_entries {
- u32 tag;
- u32 size;
- u32 bit;
- u32 length;
- u32 config;
- u32 config_id;
- u8 name[CMOS_MAX_NAME_LENGTH];
+};
memcmp() expects char *
What should change? I'm thinking memcmp() and friends.
Later revisions breaks more stuff. :\
//Peter
On Sat, 9 Aug 2008, Peter Stuge wrote:
[snip]
memcmp() expects char *
What should change? I'm thinking memcmp() and friends.
memcmp is memcmp(const void *, const void *, size_t) in libc, so I guess we should stick to that, see attached patch against r3478. Compile tested. coreinfo still runs under qemu after this patch is applied.
Later revisions breaks more stuff. :\
Yes.
r3479: pci_module output becomes erratic in coreinfo. r3482: no output at all from coreinfo on neither vga nor serial in qemu.
/ulf
On Sat, Aug 09, 2008 at 09:27:58PM +0200, Ulf Jordan wrote:
On Sat, 9 Aug 2008, Stefan Reinauer wrote:
void * is a good idea here.
I sent a patch along those lines in this thread earlier today (10:21:43 +0200 (CEST)), chaning to void * in libpayload.h and memory.c. Does it do the right thing?
If not, it is very close.
On Sat, Aug 09, 2008 at 10:21:43AM +0200, Ulf Jordan wrote:
Fix signedness problem in memcmp.
Signed-off-by: Ulf Jordan jordan@chalmers.se
Index: libpayload/include/libpayload.h
--- libpayload/include/libpayload.h (revision 3478) +++ libpayload/include/libpayload.h (arbetskopia) @@ -179,7 +179,7 @@ void *memset(void *s, int c, size_t n); void *memcpy(void *dst, const void *src, size_t n); void *memmove(void *dst, const void *src, size_t n); -int memcmp(const char *s1, const char *s2, size_t len); +int memcmp(const void *s1, const void *s2, size_t len);
/* libc/printf.c */ int snprintf(char *str, size_t size, const char *fmt, ...); Index: libpayload/libc/memory.c =================================================================== --- libpayload/libc/memory.c (revision 3478) +++ libpayload/libc/memory.c (arbetskopia) @@ -107,8 +107,8 @@
- @return If len is 0, return zero. If the areas match, return zero.
Otherwise return non-zero.*/ -int memcmp(const char *s1, const char *s2, size_t len) +int memcmp(const void *s1, const void *s2, size_t len) {
- for (; len && *s1++ == *s2++; len--) ;
- for (; len && *(char *)s1++ == *(char *)s2++; len--) ; return len;
}
Maybe cast to unsigned char or u8 instead?
I'm good with this though.
Acked-by: Peter Stuge peter@stuge.se
On Sat, Aug 09, 2008 at 09:31:47PM +0200, Peter Stuge wrote:
On Sat, Aug 09, 2008 at 10:21:43AM +0200, Ulf Jordan wrote:
Fix signedness problem in memcmp.
Signed-off-by: Ulf Jordan jordan@chalmers.se
Acked-by: Peter Stuge peter@stuge.se
r3491
On 09/08/08 21:31 +0200, Peter Stuge wrote:
On Sat, Aug 09, 2008 at 09:27:58PM +0200, Ulf Jordan wrote:
On Sat, 9 Aug 2008, Stefan Reinauer wrote:
void * is a good idea here.
I sent a patch along those lines in this thread earlier today (10:21:43 +0200 (CEST)), chaning to void * in libpayload.h and memory.c. Does it do the right thing?
If not, it is very close.
On Sat, Aug 09, 2008 at 10:21:43AM +0200, Ulf Jordan wrote:
Fix signedness problem in memcmp.
Signed-off-by: Ulf Jordan jordan@chalmers.se
Index: libpayload/include/libpayload.h
--- libpayload/include/libpayload.h (revision 3478) +++ libpayload/include/libpayload.h (arbetskopia) @@ -179,7 +179,7 @@ void *memset(void *s, int c, size_t n); void *memcpy(void *dst, const void *src, size_t n); void *memmove(void *dst, const void *src, size_t n); -int memcmp(const char *s1, const char *s2, size_t len); +int memcmp(const void *s1, const void *s2, size_t len);
/* libc/printf.c */ int snprintf(char *str, size_t size, const char *fmt, ...); Index: libpayload/libc/memory.c =================================================================== --- libpayload/libc/memory.c (revision 3478) +++ libpayload/libc/memory.c (arbetskopia) @@ -107,8 +107,8 @@
- @return If len is 0, return zero. If the areas match, return zero.
Otherwise return non-zero.*/ -int memcmp(const char *s1, const char *s2, size_t len) +int memcmp(const void *s1, const void *s2, size_t len) {
- for (; len && *s1++ == *s2++; len--) ;
- for (; len && *(char *)s1++ == *(char *)s2++; len--) ; return len;
}
Maybe cast to unsigned char or u8 instead?
Yes - we should cast to unsigned everywhere in here.
Jordan
On Sat, 9 Aug 2008, Peter Stuge wrote:
On Sat, Aug 09, 2008 at 02:09:22PM -0600, Jordan Crouse wrote:
Maybe cast to unsigned char or u8 instead?
Yes - we should cast to unsigned everywhere in here.
Here's a patch.
Looks good and compiles.
Acked-by: Ulf Jordan jordan@chalmers.se
libpayload: Use u8 for the comparison in memcmp()
Signed-off-by: Peter Stuge peter@stuge.se
Index: libc/memory.c
--- libc/memory.c (revision 3492) +++ libc/memory.c (working copy) @@ -109,6 +109,6 @@ */ int memcmp(const void *s1, const void *s2, size_t len) {
- for (; len && *(char *)s1++ == *(char *)s2++; len--) ;
- for (; len && *(u8 *)s1++ == *(u8 *)s2++; len--) ; return len;
}
Peter Stuge wrote:
On Thu, Aug 07, 2008 at 12:21:05PM +0200, svn@coreboot.org wrote:
Author: stepan Date: 2008-08-07 12:21:05 +0200 (Thu, 07 Aug 2008) New Revision: 3474
Added: trunk/payloads/libpayload/drivers/options.c Modified: trunk/payloads/libpayload/drivers/Makefile.inc trunk/payloads/libpayload/i386/coreboot.c trunk/payloads/libpayload/include/coreboot_tables.h trunk/payloads/libpayload/include/sysinfo.h Log: add get_option to libpayload, so coreboot cmos options can be queried.
Signed-off-by: Stefan Reinauer stepan@coresystems.de Acked-by: Jordan Crouse jordan.crouse@amd.com Acked-by: Peter Stuge peter@stuge.se
Sorry.
This breaks my build. I should have tested. :\
cc -m32 -Wall -Werror -fno-stack-protector -nostdinc -I./include -I/usr/lib/gcc/i686-pc-linux-gnu/4.2.2/include -c -o drivers/options.o drivers/options.c cc1: warnings being treated as errors drivers/options.c: In function 'get_option': drivers/options.c:93: warning: pointer targets in passing argument 1 of 'memcmp' differ in signedness make: *** [drivers/options.o] Error 1
From the patch:
if (memcmp(cmos_entry->name, name, len))..
+#define CB_TAG_OPTION 0x00c9 +#define CMOS_MAX_NAME_LENGTH 32 +struct cb_cmos_entries {
- u32 tag;
- u32 size;
- u32 bit;
- u32 length;
- u32 config;
- u32 config_id;
- u8 name[CMOS_MAX_NAME_LENGTH];
+};
memcmp() expects char *
What should change? I'm thinking memcmp() and friends.
Later revisions breaks more stuff. :\
Weird, it compiles fine here. I strongly suggest changing the Makefile, adding -Wno-pointer-sign. It's a dumb warning.
On 09/08/08 12:04 +0200, Stefan Reinauer wrote:
Peter Stuge wrote:
On Thu, Aug 07, 2008 at 12:21:05PM +0200, svn@coreboot.org wrote:
Author: stepan Date: 2008-08-07 12:21:05 +0200 (Thu, 07 Aug 2008) New Revision: 3474
Added: trunk/payloads/libpayload/drivers/options.c Modified: trunk/payloads/libpayload/drivers/Makefile.inc trunk/payloads/libpayload/i386/coreboot.c trunk/payloads/libpayload/include/coreboot_tables.h trunk/payloads/libpayload/include/sysinfo.h Log: add get_option to libpayload, so coreboot cmos options can be queried.
Signed-off-by: Stefan Reinauer stepan@coresystems.de Acked-by: Jordan Crouse jordan.crouse@amd.com Acked-by: Peter Stuge peter@stuge.se
Sorry.
This breaks my build. I should have tested. :\
cc -m32 -Wall -Werror -fno-stack-protector -nostdinc -I./include -I/usr/lib/gcc/i686-pc-linux-gnu/4.2.2/include -c -o drivers/options.o drivers/options.c cc1: warnings being treated as errors drivers/options.c: In function 'get_option': drivers/options.c:93: warning: pointer targets in passing argument 1 of 'memcmp' differ in signedness make: *** [drivers/options.o] Error 1
From the patch:
if (memcmp(cmos_entry->name, name, len))..
+#define CB_TAG_OPTION 0x00c9 +#define CMOS_MAX_NAME_LENGTH 32 +struct cb_cmos_entries {
- u32 tag;
- u32 size;
- u32 bit;
- u32 length;
- u32 config;
- u32 config_id;
- u8 name[CMOS_MAX_NAME_LENGTH];
+};
memcmp() expects char *
What should change? I'm thinking memcmp() and friends.
Later revisions breaks more stuff. :\
Weird, it compiles fine here. I strongly suggest changing the Makefile, adding -Wno-pointer-sign. It's a dumb warning.
Nak. This is not a dumb warning - I have seen it expose some real issues. Its not hard to make sure your code is sign correct, casting doesn't cost us anything.
And the memory compare functions should probably be taking void *.
Jordan
Jordan Crouse wrote:
Weird, it compiles fine here. I strongly suggest changing the Makefile, adding -Wno-pointer-sign. It's a dumb warning.
Nak. This is not a dumb warning - I have seen it expose some real issues.
Yes? Please exemplify
And the memory compare functions should probably be taking void *.
Just checked. Normal libc memcmp takes void. I thought libpayload functions were modelled after these. If not, that should be fixed anyways.
void * is a good idea here.
Hi Stefan!
On Sat, 9 Aug 2008, Stefan Reinauer wrote:
And the memory compare functions should probably be taking void *.
Just checked. Normal libc memcmp takes void. I thought libpayload functions were modelled after these. If not, that should be fixed anyways.
void * is a good idea here.
I sent a patch along those lines in this thread earlier today (10:21:43 +0200 (CEST)), chaning to void * in libpayload.h and memory.c. Does it do the right thing?
/ulf
-
Jordan Crouse -
Peter Stuge -
Stefan Reinauer -
Ulf Jordan