compile-i386.c: don't mix calls to write(2) with stdio
PATCH 4/10 compile-i386.c: don't mix calls to write(2) with stdio
Some versions of gcc (e.g. v4.8.2) complain about ignoring the return value of a call to 'write(2)' on line 735, since it is marked with the warn_unused_result attribute. (see also lines 745-747).
Note that this code is mixing calls to 'write' on STDOUT_FILENO with calls to stdio functions on the stdout file handle. So, in order to suppress the compiler warnings, we replace both calls to 'write' with equivalent calls to stdio functions.
---
Hi Chris,
Since there are no tests for the compile program, I ran the old version (from commit 748b856) and this version over all of the .c files in the sparse directory, saving the output to file and diff-ing the result. Unfortunately, compile segfaults on most files and the diff shows that this patch leads to a change in behaviour, caused by the lack of I/O buffer flushing prior to the segfault (all of the diffs showed missing text at the end of each new output file).
Just to confirm, I added back the 'fflush(stdout); ...' line that I removed in the third hunk below _and_ duplicated that line and placed it after the call to 'emit_atom_list(f);'. This version of compile produced (almost) exactly the same output as the original.
The reason for the almost is that three files showed a non-empty diff which seemed to indicate (a possible bug?) that a label name was being generated from a memory address. The actual addresses/label name generated in each file was different, but they were used consistently in each file. For example, the complete diff for sort.c is:
--- sort.c.out-old 2014-08-04 17:32:32.399744973 +0100
+++ sort.c.out-new 2014-08-04 17:37:04.189092702 +0100
@@ -742,7 +742,7 @@
addl $744, %esp
ret
.size merge_block_seqs, .-merge_block_seqs
-.L0xb740550c:
+.L0xb748068c:
.globl sort_list
.type sort_list, @function
sort_list:
@@ -837,7 +837,7 @@
movl %eax, 68(%esp) # .... end copy
jmp .L72
.L87: # end if
- jmp .LS0xb740550c
+ jmp .LS0xb748068c
.L86: # end if
movl 44(%esp), %eax # begin copy ..
movl %eax, 72(%esp) # .... end copyATB,
Ramsay Jones
compile-i386.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
compile-i386.c +2 -5
@@ -732,7 +732,7 @@ static void emit_insn_atom(struct function *f, struct atom *atom) atom->insn, comment[0] ? "\t\t" : "", comment); -write (STDOUT_FILENO , s,strlen (s)); +printf ("%s" , s); }static void emit_atom_list (struct function *f)
@@ -742,9 +742,7 @@ static void emit_atom_list(struct function *f) FOR_EACH_PTR(f->atom_list, atom) {switch (atom->type) {case ATOM_TEXT : { -ssize_t rc =write (STDOUT_FILENO , atom->text, - atom->text_len); - (void ) rc;/* FIXME */ +printf ("%.*s" , (int )atom->text_len, atom->text);break ; }case ATOM_INSN :
@@ -886,7 +884,6 @@ static void emit_func_post(struct symbol *sym)insn ("ret" ,NULL ,NULL ,NULL );/* output everything to stdout */ -fflush (stdout);/* paranoia; needed? */ emit_atom_list (f);/* function footer */
Signature · 1 line
2.0.0
Christopher Li
maintainerCompile-i386 is pretty much not maintained any more.
compile-i386.c static void emit_insn_atom(struct function *f, struct atom *atom) · line 735
Quoted 3 lines · Ramsay Jones
compile-i386.c +1
-1-write (STDOUT_FILENO , s,strlen (s)); +printf ("%s" , s);
This change fix nothing. "printf" can return how many bytes it is written. In the case that "write" can fail, "printf" can fail as well. e.g. disk full. This patch did not check the return value from printf either.
If we really care about return value of write, (we don't). We can do some thing like:
if (write(...)< len)
die("....");That at least check the return value.
Chris
Quoted 10 lines · Christopher Li
Compile-i386 is pretty much not maintained any more.
compile-i386.c static void emit_insn_atom(struct function *f, struct atom *atom) · line 735
Quoted 3 lines · Ramsay Jones
compile-i386.c +1
-1-write (STDOUT_FILENO , s,strlen (s)); +printf ("%s" , s);This change fix nothing. "printf" can return how many bytes it is written. In the case that "write" can fail, "printf" can fail as well. e.g. disk full. This patch did not check the return value from printf either.
I was only really fixing the compiler warning (in an side-ways fashion). (although I think it is an improvement).
If this is no longer being maintained, perhaps a patch to remove it would be better?
ATB,
Ramsay Jones
Christopher Li
maintainerQuoted 6 lines · Ramsay Jones
Quoted 3 lines
compile-i386.c static void emit_insn_atom(struct function *f, struct atom *atom) · line 735
Quoted 2 lines
compile-i386.c +1
-1-write (STDOUT_FILENO , s,strlen (s)); +printf ("%s" , s);I was only really fixing the compiler warning (in an side-ways fashion). (although I think it is an improvement).
I would just check the return value and "die" if there is an error to silence the error.
Chris
Quoted 11 lines · Christopher Li
Quoted 6 lines · Ramsay Jones
Quoted 3 lines
compile-i386.c static void emit_insn_atom(struct function *f, struct atom *atom) · line 735
Quoted 2 lines
compile-i386.c +1
-1-write (STDOUT_FILENO , s,strlen (s)); +printf ("%s" , s);I was only really fixing the compiler warning (in an side-ways fashion). (although I think it is an improvement).
I would just check the return value and "die" if there is an error to silence the error.
Hmmm, I can change the patch to do that if you prefer.
Having said that, given that this is not currently maintained, what do you think about a patch to remove it?
[Are you hoping someone will pick it up again soon?]
Let me know which patch you would like to see.
ATB,
Ramsay Jones