Am 19.09.2011 02:01 schrieb Stefan Tauner:
Push those changes forward where needed to prevent new sign conversion warnings where possible.
Thanks for your patch.
this is a crude rework of the reverted patch. i have tried to push unsignedness wherever possible, but there are some problems: ffs, min, max and some library/system calls require signed ints or return them or both.
We might want to provide type-safe min/max macros for those cases. An example is at http://gcc.gnu.org/onlinedocs/gcc/Typeof.html Not essential for this patch, but might be nice to have anyway. Later.
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
diff --git a/flash.h b/flash.h index 535c1b8..e7bfd4e 100644 --- a/flash.h +++ b/flash.h @@ -121,8 +121,10 @@ struct flashchip {
int (*probe) (struct flashchip *flash);
- /* Delay after "enter/exit ID mode" commands in microseconds. */
- int probe_timing;
- /* Delay after "enter/exit ID mode" commands in microseconds.
* NB: negative values have special meanings, see TIMING_* below.*/- signed int probe_timing;
AFAICS the C standard defines that int is always signed. The only exception is char which is implementation-defined. Please kill signed here, but keep your comment.
/* * Erase blocks and associated erase function. Any chip erase function diff --git a/ft2232_spi.c b/ft2232_spi.c index 8ab89fa..8f00685 100644 --- a/ft2232_spi.c +++ b/ft2232_spi.c @@ -106,10 +106,10 @@ static const char *get_ft2232_vendorname(int ft2232_vid, int ft2232_type) }
static int send_buf(struct ftdi_context *ftdic, const unsigned char *buf,
int size)
unsigned int size){ int r;
- r = ftdi_write_data(ftdic, (unsigned char *) buf, size);
- r = ftdi_write_data(ftdic, (unsigned char *) buf, (int)size);
This cast is a sign that something is odd. Can you keep size as a signed int?
if (r < 0) { msg_perr("ftdi_write_data: %d, %s\n", r, ftdi_get_error_string(ftdic)); @@ -119,19 +119,19 @@ static int send_buf(struct ftdi_context *ftdic, const unsigned char *buf, }
static int get_buf(struct ftdi_context *ftdic, const unsigned char *buf,
int size)
unsigned int size){ int r;
while (size > 0) {
r = ftdi_read_data(ftdic, (unsigned char *) buf, size);
r = ftdi_read_data(ftdic, (unsigned char *) buf, (int)size);
Same here.
if (r < 0) { msg_perr("ftdi_read_data: %d, %s\n", r, ftdi_get_error_string(ftdic)); return 1; }
buf += r;size -= r;
buf += (unsigned int)r;size -= (unsigned int)r;
...
} return 0; } @@ -332,9 +332,10 @@ static int ft2232_spi_send_command(unsigned int writecnt, unsigned int readcnt, struct ftdi_context *ftdic = &ftdic_context; static unsigned char *buf = NULL; /* failed is special. We use bitwise ops, but it is essentially bool. */
- int i = 0, ret = 0, failed = 0;
- int bufsize;
- static int oldbufsize = 0;
int ret = 0, failed = 0;
unsigned int i = 0;
unsigned int bufsize;
static unsigned int oldbufsize = 0;
if (writecnt > 65536 || readcnt > 65536) return SPI_INVALID_LENGTH;
get_buf and send_buf calls should be checked for size>=INT_MAX and the cast should happen in the call, not in get_buf/send_buf.
diff --git a/serprog.c b/serprog.c index 9d554c9..94ae1c6 100644 --- a/serprog.c +++ b/serprog.c @@ -707,7 +707,7 @@ uint8_t serprog_chip_readb(const chipaddr addr) /* Local version that really does the job, doesn't care of max_read_n. */ static void sp_do_read_n(uint8_t * buf, const chipaddr addr, size_t len) {
- int rd_bytes = 0;
- unsigned int rd_bytes = 0; unsigned char sbuf[6]; msg_pspew("%s: addr=0x%lx len=%lu\n", __func__, addr, (unsigned long)len); /* Stream the read-n -- as above. */
@@ -725,7 +725,7 @@ static void sp_do_read_n(uint8_t * buf, const chipaddr addr, size_t len) int r = read(sp_fd, buf + rd_bytes, len - rd_bytes); if (r <= 0) sp_die("Error: cannot read read-n data");
rd_bytes += r;
rd_bytes += (unsigned int)r;
Odd cast, but safe. You correctly analyzed that situation.
} while (rd_bytes != len); return; }
The flashrom.c changes were really hard to verify, but that's not your fault. The code in there is not easy to understand, but you got it exactly right. With the above comments this is Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Regards, Carl-Daniel
On Sun, 13 Nov 2011 23:23:39 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
- /* Delay after "enter/exit ID mode" commands in microseconds. */
- int probe_timing;
- /* Delay after "enter/exit ID mode" commands in microseconds.
* NB: negative values have special meanings, see TIMING_* below.*/- signed int probe_timing;
AFAICS the C standard defines that int is always signed. The only exception is char which is implementation-defined. Please kill signed here, but keep your comment.
this was on purpose to make it really, really obvious to anyone reading the code that this is not just an unsigned delay (even if he ignores the comment). :) because you know... one could think that and change it in a patch which gets acked and committed by accident and produce really odd bugs ;)
will review the other comments later, thanks!
On Sun, 13 Nov 2011 23:23:39 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 19.09.2011 02:01 schrieb Stefan Tauner:
Push those changes forward where needed to prevent new sign conversion warnings where possible.
Thanks for your patch.
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
diff --git a/flash.h b/flash.h index 535c1b8..e7bfd4e 100644 --- a/flash.h +++ b/flash.h @@ -121,8 +121,10 @@ struct flashchip {
int (*probe) (struct flashchip *flash);
- /* Delay after "enter/exit ID mode" commands in microseconds. */
- int probe_timing;
- /* Delay after "enter/exit ID mode" commands in microseconds.
* NB: negative values have special meanings, see TIMING_* below.*/- signed int probe_timing;
AFAICS the C standard defines that int is always signed. The only exception is char which is implementation-defined. Please kill signed here, but keep your comment.
see other mail, unchanged in attached patch.
/* * Erase blocks and associated erase function. Any chip erase function diff --git a/ft2232_spi.c b/ft2232_spi.c index 8ab89fa..8f00685 100644 --- a/ft2232_spi.c +++ b/ft2232_spi.c @@ -106,10 +106,10 @@ static const char *get_ft2232_vendorname(int ft2232_vid, int ft2232_type) }
static int send_buf(struct ftdi_context *ftdic, const unsigned char *buf,
int size)
unsigned int size){ int r;
- r = ftdi_write_data(ftdic, (unsigned char *) buf, size);
- r = ftdi_write_data(ftdic, (unsigned char *) buf, (int)size);
This cast is a sign that something is odd. Can you keep size as a signed int?
reverted here and below...
if (r < 0) { msg_perr("ftdi_write_data: %d, %s\n", r, ftdi_get_error_string(ftdic)); @@ -119,19 +119,19 @@ static int send_buf(struct ftdi_context *ftdic, const unsigned char *buf, }
static int get_buf(struct ftdi_context *ftdic, const unsigned char *buf,
int size)
unsigned int size){ int r;
while (size > 0) {
r = ftdi_read_data(ftdic, (unsigned char *) buf, size);
r = ftdi_read_data(ftdic, (unsigned char *) buf, (int)size);Same here.
if (r < 0) { msg_perr("ftdi_read_data: %d, %s\n", r, ftdi_get_error_string(ftdic)); return 1; }
buf += r;size -= r;
buf += (unsigned int)r;size -= (unsigned int)r;...
} return 0; } @@ -332,9 +332,10 @@ static int ft2232_spi_send_command(unsigned int writecnt, unsigned int readcnt, struct ftdi_context *ftdic = &ftdic_context; static unsigned char *buf = NULL; /* failed is special. We use bitwise ops, but it is essentially bool. */
- int i = 0, ret = 0, failed = 0;
- int bufsize;
- static int oldbufsize = 0;
int ret = 0, failed = 0;
unsigned int i = 0;
unsigned int bufsize;
static unsigned int oldbufsize = 0;
if (writecnt > 65536 || readcnt > 65536) return SPI_INVALID_LENGTH;
get_buf and send_buf calls should be checked for size>=INT_MAX and the cast should happen in the call, not in get_buf/send_buf.
i am not sure this is what we want. mainly... because (the buffer handling in) ft2232_spi.c is slightly awful :) various "bufs" (well, 2), some of them static (ok, only the one in send_command, but still this is messier than expected for such a simple driver), the two ~equivalent failed/ret variables in send_command etc. make it a bit unreadable.
we need to check for failure of the buffer methods anyway, so i dont see a reason to do the size checks in the caller(s).
i have reverted the whole file now, because i am not sure how to proceed and wanted to publish what i have. not much changed, but i had to rebase it due to the addition of the opaque framework and hwseq.
[…] With the above comments this is Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
should i commit the attached version?
Am 22.11.2011 23:09 schrieb Stefan Tauner:
(the buffer handling in) ft2232_spi.c is slightly awful :) various "bufs" (well, 2), some of them static (ok, only the one in send_command, but still this is messier than expected for such a simple driver), the two ~equivalent failed/ret variables in send_command etc. make it a bit unreadable.
Indeed, this file could use some cleanup.
we need to check for failure of the buffer methods anyway, so i dont see a reason to do the size checks in the caller(s).
i have reverted the whole file now, because i am not sure how to proceed and wanted to publish what i have. not much changed, but i had to rebase it due to the addition of the opaque framework and hwseq.
[…] With the above comments this is Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
should i commit the attached version?
Please do, the ack is still valid.
I wonder whether we should enable the sign warnings once the cleanups and remaining conversions are in. The big problem might be platform (OS/library version) dependent changes which we don't know of, but OTOH we won't know until we try it.
Regards, Carl-Daniel
On Wed, 23 Nov 2011 00:19:32 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 22.11.2011 23:09 schrieb Stefan Tauner:
should i commit the attached version?
Please do, the ack is still valid.
thanks, r1470
I wonder whether we should enable the sign warnings once the cleanups and remaining conversions are in. The big problem might be platform (OS/library version) dependent changes which we don't know of, but OTOH we won't know until we try it.
i have been playing around with -Wsign-conversion for this patch... and at least on my gcc version it is way over-sensitive, for example ichspi.c: In function ‘ich7_run_opcode’: ichspi.c:742: warning: negative integer implicitly converted to unsigned type
that is: temp32 = REGREAD32(ICH7_REG_SPIA) & ~0x00FFFFFF;
together with -Werror -Wsign-conversion will probably explode somewhere even if we try hard to get it right on dev platforms.
-
Carl-Daniel Hailfinger -
Stefan Tauner