This patch fixes a bunch of compiler warnings I ran into while building an MCP55 target using gcc 4.1.1.
A few are real bugs, like the misspelled "default" in raminit_f.c and the truncated 16-bit argument to delayx() in mcp55_early_setup_car.c. The uninitialized variables in raminit_f_dqs.c are potential bugs.
gcc 4.1.x is famously paranoid about certain things, and some of its warnings can only be considered compiler bugs, so I ignored those. The minor warnings I fixed are mainly improper int/pointer casts and unused variables.
Signed-off-by: Ed Swierk eswierk@arastra.com
--Ed
Ed Swierk wrote:
This patch fixes a bunch of compiler warnings I ran into while building an MCP55 target using gcc 4.1.1.
A few are real bugs, like the misspelled "default" in raminit_f.c and the truncated 16-bit argument to delayx() in mcp55_early_setup_car.c. The uninitialized variables in raminit_f_dqs.c are potential bugs.
Ack.
gcc 4.1.x is famously paranoid about certain things, and some of its warnings can only be considered compiler bugs, so I ignored those. The minor warnings I fixed are mainly improper int/pointer casts and unused variables.
Hmmm. You know that the signedness of char is not defined?
Besides that, we definitely should enable -fno-strict-aliasing in the gcc flags until we have audited all casts.
--- LinuxBIOSv2-2540.orig/util/options/build_opt_tbl.c +++ LinuxBIOSv2-2540/util/options/build_opt_tbl.c @@ -332,7 +332,7 @@ int main(int argc, char **argv) fprintf(stderr, "Error - Length is to long in line \n%s\n",line); exit(1); }
if (!is_ident(ce->name)) {
if (!is_ident((char *) ce->name)) { fprintf(stderr, "Error - Name %s is an invalid identifier in line\n %s\n", ce->name, line);
Nack. Fix struct cmos_entries instead. is_ident_* needs a lot of cleaning, too.
@@ -341,7 +341,7 @@ int main(int argc, char **argv) /* put in the record type */ ce->tag=LB_TAG_OPTION; /* calculate and save the record length */
len=strlen(ce->name)+1;
len=strlen((char *) ce->name)+1;
ditto
@@ -540,7 +540,7 @@ int main(int argc, char **argv) if (ce->config == 'r') { continue; }
if (!is_ident(ce->name)) {
if (!is_ident((char *) ce->name)) { fprintf(stderr, "Invalid identifier: %s\n", ce->name); exit(1);
ditto
--- LinuxBIOSv2-2540.orig/src/cpu/amd/model_fxx/model_fxx_init.c +++ LinuxBIOSv2-2540/src/cpu/amd/model_fxx/model_fxx_init.c @@ -17,6 +17,7 @@ #include <cpu/x86/pae.h> #include <pc80/mc146818rtc.h> #include <cpu/x86/lapic.h> +#include <part/hard_reset.h>
#include "../../../northbridge/amd/amdk8/amdk8.h"
@@ -473,7 +474,7 @@ static void amd_set_name_string_f(device unsigned nN; unsigned unknown = 1;
- uint8_t str[48];
char str[48]; uint32_t *p;
msr_t msr;
Please remove the magic constant 48 when you change this line. Same for all other magic 48.
@@ -533,7 +534,7 @@ static void amd_set_name_string_f(device #endif }
- p = str;
- p = (uint32_t *) str; for(i=0;i<6;i++) { msr.lo = *p; p++; msr.hi = *p; p++; wrmsr(0xc0010030+i, msr);
While I agree with this cast, gcc is free to break this code completely unless we use -fno-strict-aliasing.
Index: LinuxBIOSv2-2540/src/northbridge/amd/amdk8/coherent_ht_car.c
--- LinuxBIOSv2-2540.orig/src/northbridge/amd/amdk8/coherent_ht_car.c +++ LinuxBIOSv2-2540/src/northbridge/amd/amdk8/coherent_ht_car.c @@ -1662,7 +1662,6 @@ static int apply_cpu_errata_fixes(unsign int needs_reset = 0; for(node = 0; node < nodes; node++) { device_t dev;
uint32_t cmd;#if K8_REV_F_SUPPORT == 0 if (is_cpu_pre_c0()) { Index: LinuxBIOSv2-2540/src/northbridge/amd/amdk8/misc_control.c =================================================================== --- LinuxBIOSv2-2540.orig/src/northbridge/amd/amdk8/misc_control.c +++ LinuxBIOSv2-2540/src/northbridge/amd/amdk8/misc_control.c @@ -110,7 +110,10 @@ static void misc_control_init(struct dev { uint32_t cmd, cmd_ref; int needs_reset;
- struct device *f0_dev, *f2_dev;
- struct device *f0_dev;
+#if K8_REV_F_SUPPORT == 0
struct device *f2_dev;+#endif
printk_debug("NB: Function 3 Misc Control.. "); needs_reset = 0; Index: LinuxBIOSv2-2540/src/northbridge/amd/amdk8/raminit_f.c =================================================================== --- LinuxBIOSv2-2540.orig/src/northbridge/amd/amdk8/raminit_f.c +++ LinuxBIOSv2-2540/src/northbridge/amd/amdk8/raminit_f.c @@ -2506,7 +2506,7 @@ static void set_misc_timing(const struct case 0x00: dwordx = 0x002b2220; //x8 double Rank break;
defalut:
default: dwordx = 0x002a2220; //x8 single Rank and double Rank mixed } } else if((meminfo->x4_mask == 0) && (meminfo->x16_mask == 0x01) && (meminfo->single_rank_mask == 0x01)) {
Nice spot.
@@ -2865,7 +2865,7 @@ static void sdram_enable(int controllers
/* Before enabling memory start the memory clocks */ for(i = 0; i < controllers; i++) {
uint32_t dtl, dch;
uint32_t dch;@@ -2938,7 +2938,7 @@ static void sdram_enable(int controllers }
for(i = 0; i < controllers; i++) {
uint32_t dcl, dch, dcm;
uint32_t dcl, dcm;Index: LinuxBIOSv2-2540/src/northbridge/amd/amdk8/raminit_f_dqs.c
--- LinuxBIOSv2-2540.orig/src/northbridge/amd/amdk8/raminit_f_dqs.c +++ LinuxBIOSv2-2540/src/northbridge/amd/amdk8/raminit_f_dqs.c @@ -551,13 +551,15 @@ static unsigned TrainRcvrEn(const struct
unsigned TestAddr0, TestAddr0B, TestAddr1, TestAddr1B;
- unsigned CurrRcvrCHADelay;
unsigned CurrRcvrCHADelay = 0;
unsigned tmp;
unsigned is_Width128 = sysinfo->meminfo[ctrl->node_id].is_Width128;
+#if K8_REV_F_SUPPORT_F0_F1_WORKAROUND == 1 unsigned cpu_f0_f1; +#endif
if(Pass == DQS_FIRST_PASS) { InitDQSPos4RcvrEn(ctrl); @@ -1205,6 +1207,7 @@ static unsigned TrainDQSPos(const struct
LastTest = DQS_FAIL; RnkDlySeqPassMax = 0;
RnkDlySeqPassMin = 0;Index: LinuxBIOSv2-2540/src/pc80/mc146818rtc.c
--- LinuxBIOSv2-2540.orig/src/pc80/mc146818rtc.c +++ LinuxBIOSv2-2540/src/pc80/mc146818rtc.c @@ -123,8 +123,10 @@ static void rtc_set_checksum(int range_s
void rtc_init(int invalid) { +#if HAVE_OPTION_TABLE unsigned char x; int cmos_invalid, checksum_invalid; +#endif
printk_debug("RTC Init\n");
Index: LinuxBIOSv2-2540/src/ram/ramtest.c
--- LinuxBIOSv2-2540.orig/src/ram/ramtest.c +++ LinuxBIOSv2-2540/src/ram/ramtest.c @@ -94,7 +94,6 @@ static void ram_verify(unsigned long sta
void ram_check(unsigned long start, unsigned long stop) {
- int result; /*
- This is much more of a "Is my DRAM properly configured?"
- test than a "Is my DRAM faulty?" test. Not all bits
Index: LinuxBIOSv2-2540/src/southbridge/nvidia/mcp55/mcp55_aza.c
--- LinuxBIOSv2-2540.orig/src/southbridge/nvidia/mcp55/mcp55_aza.c +++ LinuxBIOSv2-2540/src/southbridge/nvidia/mcp55/mcp55_aza.c @@ -27,6 +27,7 @@ #include <device/pci_ids.h> #include <device/pci_ops.h> #include <arch/io.h> +#include <delay.h> #include "mcp55.h"
static int set_bits(uint8_t *port, uint32_t mask, uint32_t val) @@ -228,7 +229,7 @@ static void aza_init(struct device *dev) if(!res) return;
- base =(uint8_t *) res->base;
base =(uint8_t *) ((uint32_t) res->base); printk_debug("base = %08x\n", base);
codec_mask = codec_detect(base);
Are you really sure you want to cast a uint64_t to uint32_t to uint8_t * ?
Index: LinuxBIOSv2-2540/src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c
--- LinuxBIOSv2-2540.orig/src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c +++ LinuxBIOSv2-2540/src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c @@ -175,7 +175,8 @@ static void mcp55_early_pcie_setup(unsig pci_write_config32(dev, 0xe4, dword);
// need to wait 100ms
- delayx(1000);
- for (i=0; i<10; i++)
delayx(100);}
static void mcp55_early_setup(unsigned mcp55_num, unsigned *busn, unsigned *devn, unsigned *io_base, unsigned *pci_e_x)
Nack. You increased the delay by a factor of 10.
Index: LinuxBIOSv2-2540/src/southbridge/nvidia/mcp55/mcp55_lpc.c
--- LinuxBIOSv2-2540.orig/src/southbridge/nvidia/mcp55/mcp55_lpc.c +++ LinuxBIOSv2-2540/src/southbridge/nvidia/mcp55/mcp55_lpc.c @@ -243,7 +243,6 @@ static void lpc_init(device_t dev) static void mcp55_lpc_read_resources(device_t dev) { struct resource *res;
unsigned long index;
/* Get the normal pci resources of this device */ pci_dev_read_resources(dev); // We got one for APIC, or one more for TRAP
Index: LinuxBIOSv2-2540/src/southbridge/nvidia/mcp55/mcp55_nic.c
--- LinuxBIOSv2-2540.orig/src/southbridge/nvidia/mcp55/mcp55_nic.c +++ LinuxBIOSv2-2540/src/southbridge/nvidia/mcp55/mcp55_nic.c @@ -28,6 +28,7 @@ #include <device/pci_ids.h> #include <device/pci_ops.h> #include <arch/io.h> +#include <delay.h> #include "mcp55.h"
static int phy_read(uint8_t *base, unsigned phy_addr, unsigned phy_reg) @@ -55,7 +56,7 @@ static int phy_read(uint8_t *base, unsig
}
-static int phy_detect(uint8_t *base) +static void phy_detect(uint8_t *base) { uint32_t dword; int i; @@ -94,7 +95,6 @@ static int phy_detect(uint8_t *base) } static void nic_init(struct device *dev) {
- uint32_t dword, old; uint32_t mac_h, mac_l; int eeprom_valid = 0; struct southbridge_nvidia_mcp55_config *conf;
@@ -108,7 +108,7 @@ static void nic_init(struct device *dev)
if(!res) return;
- base = res->base;
base = (uint8_t *) ((uint32_t) res->base);
phy_detect(base);
@@ -154,8 +154,7 @@ static void nic_init(struct device *dev) } // if that is invalid we will read that from romstrap if(!eeprom_valid) {
unsigned long mac_pos;mac_pos = 0xffffffd0; // refer to romstrap.inc and romstrap.lds
uint32_t *mac_pos = (uint32_t *) 0xffffffd0; // refer to romstrap.inc and romstrap.lds
Hmmm. I have to reread that.
Regards, Carl-Daniel
On 2/1/07, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Hmmm. You know that the signedness of char is not defined?
Yes, but if functions like strlen() take a char *, then shouldn't the parameter be signed rather than unsigned?
Besides that, we definitely should enable -fno-strict-aliasing in the gcc flags until we have audited all casts.
Agreed.
Please remove the magic constant 48 when you change this line. Same for all other magic 48.
Oh boy. You're asking me to understand the code I'm tidying? :-)
Index: LinuxBIOSv2-2540/src/southbridge/nvidia/mcp55/mcp55_aza.c
--- LinuxBIOSv2-2540.orig/src/southbridge/nvidia/mcp55/mcp55_aza.c +++ LinuxBIOSv2-2540/src/southbridge/nvidia/mcp55/mcp55_aza.c @@ -27,6 +27,7 @@ #include <device/pci_ids.h> #include <device/pci_ops.h> #include <arch/io.h> +#include <delay.h> #include "mcp55.h"
static int set_bits(uint8_t *port, uint32_t mask, uint32_t val) @@ -228,7 +229,7 @@ static void aza_init(struct device *dev) if(!res) return;
base =(uint8_t *) res->base;
base =(uint8_t *) ((uint32_t) res->base); printk_debug("base = %08x\n", base); codec_mask = codec_detect(base);Are you really sure you want to cast a uint64_t to uint32_t to uint8_t * ?
I'm not sure. The current implementation truncates base to 32 bits, so I suppose there ought to be a check that base doesn't exceed 0xffffffff. But in this case the double-cast is still necessary to avoid gcc's complaint about truncating while casting to a pointer.
=================================================================== --- LinuxBIOSv2-2540.orig/src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c +++ LinuxBIOSv2-2540/src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c @@ -175,7 +175,8 @@ static void mcp55_early_pcie_setup(unsig pci_write_config32(dev, 0xe4, dword);
// need to wait 100ms
delayx(1000);
for (i=0; i<10; i++)delayx(100);}
static void mcp55_early_setup(unsigned mcp55_num, unsigned *busn, unsigned *devn, unsigned *io_base, unsigned *pci_e_x)
Nack. You increased the delay by a factor of 10.
This alters the existing behavior, yes. But if the comments in the code are to be believed, delayx(1) waits 100 usec, so for a 100-msec delay we have to call delayx(100) 10 times.
--Ed
Ed Swierk wrote:
On 2/1/07, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Hmmm. You know that the signedness of char is not defined?
Yes, but if functions like strlen() take a char *, then shouldn't the parameter be signed rather than unsigned?
For functions like strlen, we have no choice but to use naked char. So yes, my complaint was superfluous in this case.
Besides that, we definitely should enable -fno-strict-aliasing in the gcc flags until we have audited all casts.
Agreed.
Please remove the magic constant 48 when you change this line. Same for all other magic 48.
Oh boy. You're asking me to understand the code I'm tidying? :-)
Yes. Sacrificing a chicken would be nice, too.
Index: LinuxBIOSv2-2540/src/southbridge/nvidia/mcp55/mcp55_aza.c
--- LinuxBIOSv2-2540.orig/src/southbridge/nvidia/mcp55/mcp55_aza.c +++ LinuxBIOSv2-2540/src/southbridge/nvidia/mcp55/mcp55_aza.c @@ -27,6 +27,7 @@ #include <device/pci_ids.h> #include <device/pci_ops.h> #include <arch/io.h> +#include <delay.h> #include "mcp55.h"
static int set_bits(uint8_t *port, uint32_t mask, uint32_t val) @@ -228,7 +229,7 @@ static void aza_init(struct device *dev) if(!res) return;
base =(uint8_t *) res->base;
base =(uint8_t *) ((uint32_t) res->base); printk_debug("base = %08x\n", base); codec_mask = codec_detect(base);Are you really sure you want to cast a uint64_t to uint32_t to uint8_t
- ?
I'm not sure. The current implementation truncates base to 32 bits, so I suppose there ought to be a check that base doesn't exceed 0xffffffff. But in this case the double-cast is still necessary to avoid gcc's complaint about truncating while casting to a pointer.
Agreed.
===================================================================
LinuxBIOSv2-2540.orig/src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c
+++
LinuxBIOSv2-2540/src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c
@@ -175,7 +175,8 @@ static void mcp55_early_pcie_setup(unsig pci_write_config32(dev, 0xe4, dword);
// need to wait 100ms
delayx(1000);
for (i=0; i<10; i++)delayx(100);}
static void mcp55_early_setup(unsigned mcp55_num, unsigned *busn,
unsigned *devn, unsigned *io_base, unsigned *pci_e_x)
Nack. You increased the delay by a factor of 10.
This alters the existing behavior, yes. But if the comments in the code are to be believed, delayx(1) waits 100 usec, so for a 100-msec delay we have to call delayx(100) 10 times.
The code says something different.
static void delayx(uint8_t value) { int i; for(i=0;i<0x8000;i++) { outb(value, 0x80); } }
The value is the POST code and has nothing to do with the delay. So your patch changed the POST code from E8 (1000 mod 256) to 64 (100) and called the delay function ten times.
Regards, Carl-Daniel
On 2/1/07, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
The code says something different.
static void delayx(uint8_t value) { int i; for(i=0;i<0x8000;i++) { outb(value, 0x80); } }
The value is the POST code and has nothing to do with the delay. So your patch changed the POST code from E8 (1000 mod 256) to 64 (100) and called the delay function ten times.
So delayx() actually sets the POST code by writing some value 32768 times, has nothing to do with delay, and the comments about delay are completely irrelevant? My brain just exploded.
--Ed
static void delayx(uint8_t value) { int i; for(i=0;i<0x8000;i++) { outb(value, 0x80); } }
The value is the POST code and has nothing to do with the delay. So your patch changed the POST code from E8 (1000 mod 256) to 64 (100) and called the delay function ten times.
So delayx() actually sets the POST code by writing some value 32768 times, has nothing to do with delay, and the comments about delay are completely irrelevant? My brain just exploded.
Oh it certainly causes a delay -- a huge delay, irrespective of its parameter. A few millis at least.
Segher
Ed Swierk wrote:
On 2/1/07, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
The code says something different.
static void delayx(uint8_t value) { int i; for(i=0;i<0x8000;i++) { outb(value, 0x80); } }
The value is the POST code and has nothing to do with the delay. So your patch changed the POST code from E8 (1000 mod 256) to 64 (100) and called the delay function ten times.
So delayx() actually sets the POST code by writing some value 32768 times, has nothing to do with delay, and the comments about delay are completely irrelevant? My brain just exploded.
Prepare for more explosions. The POST code is written 32768 times and a side effect of writing to an I/O port on x86 is that it takes ~3 usec depending on whom you ask. Why writing the POST code, you ask? Simple. Port 0x80 is probably the one I/O port with the least side effects.
Regards, Carl-Daniel
Prepare for more explosions. The POST code is written 32768 times and a side effect of writing to an I/O port on x86 is that it takes ~3 usec depending on whom you ask. Why writing the POST code, you ask? Simple. Port 0x80 is probably the one I/O port with the least side effects.
There's also 0xeb, which is guaranteed to be empty (which makes it even slower, at least on actual ISA systems).
Segher
On 02/01/2007 01:54 PM, Carl-Daniel Hailfinger wrote:
Ed Swierk wrote:
static int set_bits(uint8_t *port, uint32_t mask, uint32_t val) @@ -228,7 +229,7 @@ static void aza_init(struct device *dev) if(!res) return;
base =(uint8_t *) res->base;
base =(uint8_t *) ((uint32_t) res->base); printk_debug("base = %08x\n", base); codec_mask = codec_detect(base);Are you really sure you want to cast a uint64_t to uint32_t to uint8_t
- ?
I'm not sure. The current implementation truncates base to 32 bits, so I suppose there ought to be a check that base doesn't exceed 0xffffffff. But in this case the double-cast is still necessary to avoid gcc's complaint about truncating while casting to a pointer.
Agreed.
Generally, everyone wants to be able to deal with 64-bit resources. The 32-bit linuxbios does not allow to do it easily. Potentially, it will migrate to 64-bit on some platforms. Such double cast will create a bug in 64-bit mode. I think you should either suppress the warning in the command line or make a function or macro that casts integers to pointers. Do not do (uint32_t).
---------------------------- - uint8_t str[48]; + char str[48]; uint32_t *p;
msr_t msr; @@ -533,7 +534,7 @@ static void amd_set_name_string_f(device #endif }
- p = str; + p = (uint32_t *) str; for(i=0;i<6;i++) { msr.lo = *p; p++; msr.hi = *p; p++; wrmsr(0xc0010030+i, msr); ---------------------------- Do not do this unless you are sure that ((char)-1)-((uint8_t)-1)==0; Sign extension in 'msr.lo = *p' may be important.
Regards,
Roman
Here's a fix for the one warning we can all agree is a bug. We can deal with the other issues separately.
Signed-off-by: Ed Swierk eswierk@arastra.com
--Ed
On Thu, Feb 01, 2007 at 01:17:50PM -0800, Ed Swierk wrote:
Here's a fix for the one warning we can all agree is a bug. We can deal with the other issues separately.
Good idea. Committed.
Uwe.
Here's a fix for the one warning we can all agree is a bug. We can deal with the other issues separately.
SW5kZXg6IExpbnV4QklPU3YyLTI1NDAvc3JjL25vcnRoYnJpZGdlL2FtZC9hbWRrOC9yYW 1pbml0 X2YuYwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT 09PT09 PT09PT09PT09PT09PT09Ci0tLSBMaW51eEJJT1N2Mi0yNTQwLm9yaWcvc3JjL25vcnRoYn JpZGdl L2FtZC9hbWRrOC9yYW1pbml0X2YuYworKysgTGludXhCSU9TdjItMjU0MC9zcmMvbm9ydG hicmlk Z2UvYW1kL2FtZGs4L3JhbWluaXRfZi5jCkBAIC0yNTA2LDcgKzI1MDYsNyBAQCBzdGF0aW Mgdm9p ZCBzZXRfbWlzY190aW1pbmcoY29uc3Qgc3RydWN0CiAgICAgICAgICAgICAgICAgICAgIC AgICAg ICAgICAgIGNhc2UgMHgwMDoKICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIC AgICAg ICBkd29yZHggPSAweDAwMmIyMjIwOyAvL3g4IGRvdWJsZSBSYW5rCiAgICAgICAgICAgIC AgICAg ICAgICAgICAgICAgICAgICAgICAgICAgYnJlYWs7Ci0gICAgICAgICAgICAgICAgICAgIC AgICAg ICAgICAgIGRlZmFsdXQ6CisgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGRlZm F1bHQ6 CiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgZHdvcmR4ID0gMH gwMDJh MjIyMDsgLy94OCBzaW5nbGUgUmFuayBhbmQgZG91YmxlIFJhbmsgbWl4ZWQKICAgICAgIC AgICAg ICAgICAgICAgICAgICAgICAgICAgfQogICAgICAgICAgICAgICAgICAgICAgICAgfSBlbH NlIGlm KChtZW1pbmZvLT54NF9tYXNrID09IDApICYmIChtZW1pbmZvLT54MTZfbWFzayA9PSAweD AxKSAm JiAobWVtaW5mby0+c2luZ2xlX3JhbmtfbWFzayA9PSAweDAxKSkgewo=
Well I'm not sure I agree this is a good idea? :-)
Segher
I think you should either suppress the warning in the command line or make a function or macro that casts integers to pointers.
Actually, you shouldn't use pointers to represent system resources at all, that's just crazy.
Segher
On 1-feb-2007, at 20:46, Ed Swierk wrote:
On 2/1/07, Carl-Daniel Hailfinger <c-d.hailfinger.devel. 2006@gmx.net> wrote:
Hmmm. You know that the signedness of char is not defined?
Yes, but if functions like strlen() take a char *, then shouldn't the parameter be signed rather than unsigned?
No; plain char is unsigned on almost all mainstream architectures.
Segher
Hmmm. You know that the signedness of char is not defined?
It is defined, but implementation-defined. Maybe you mean that "plain" char is a separate type from both signed char and unsigned char?
Besides that, we definitely should enable -fno-strict-aliasing in the gcc flags until we have audited all casts.
Well certainly as long as GCC keeps spitting warnings about this, it almost never gets these warnings wrong.
Segher
Segher Boessenkool wrote:
Hmmm. You know that the signedness of char is not defined?
It is defined, but implementation-defined. Maybe you mean that "plain" char is a separate type from both signed char and unsigned char?
Mostly. I wanted to say that the signedness of char is implementation-defined and differs between the linux targets on different architectures (see S390 vs. the rest for an example).
Besides that, we definitely should enable -fno-strict-aliasing in the gcc flags until we have audited all casts.
Well certainly as long as GCC keeps spitting warnings about this, it almost never gets these warnings wrong.
I don't know whether it is spitting warnings about aliasing, but we're violating the aliasing rules described in the gcc man page.
Regards, Carl-Daniel
Well certainly as long as GCC keeps spitting warnings about this, it almost never gets these warnings wrong.
I don't know whether it is spitting warnings about aliasing, but we're violating the aliasing rules described in the gcc man page.
Described in the C standard, even. There's a GCC option to make it complain about suspicious pointer casting; I believe it's called -Wstrict-aliasing? It helps a lot, we should have it enabled :-)
Segher
-
Carl-Daniel Hailfinger -
Ed Swierk -
Roman Kononov -
Segher Boessenkool -
Uwe Hermann