I think that this change is masking the true bug, and suspect that the true
bug will return when C compiler's optimiser is turned on. I have no knowledge
of x86 assembly language, so can't follow the logic of what the compiler is
generating, but I can see that the assembly code generated for handle_flags
differs with the application of this patch. Specifically, it appears that
when the patch is added, something extra is added to the stack. Specifically:
@@ -396,246 +396,262 @@ handle_flags:
testl %eax, %eax
je .L27
movl 12(%ebp), %eax
movl (%eax), %eax
cmpl -8(%ebp), %eax
jbe .L27
.LBB5:
- .loc 1 166 0
+ .loc 1 168 0
+ movl 12(%ebp), %eax
+ movl 8(%eax), %eax
+ shrl $2, %eax
+ andl $1, %eax
+ testl %eax, %eax
+ je .L28
+ .loc 1 169 0
movl 8(%ebp), %eax
movl 168(%eax), %eax
- addl $32, %eax
+ addl $132, %eax
movl (%eax), %eax
movl %eax, -12(%ebp)
- .loc 1 168 0
+ jmp .L29
+.L28:
+ .loc 1 172 0
+ movl 8(%ebp), %eax
+ movl 168(%eax), %eax
+ addl $124, %eax
+ movl (%eax), %eax
+ movl %eax, -12(%ebp)
+.L29:
+ .loc 1 175 0
pushl $0
movl 12(%ebp), %eax
movl -8(%ebp), %edx
movl (%eax), %eax
subl %edx, %eax
pushl %eax
pushl -12(%ebp)
pushl 8(%ebp)
call string_repeat@PLT
I assume that the thing added to the stack is a pointer to the generated
string "0" or " ", and that the bug goes away because there happens to be
a GC run triggered inside string_repeat, and with the temporary on the
stack it doesn't get garbage collected.
This is a hunch. But as far as I can make out there is no semantic difference
in the change you made to the C, so it should not have changed anything.
Nicholas Clark
>> - STRING *fill = CONST_STRING(interpreter, info->flags &
>> FLAG_ZERO ? "0" : " ");
>
> I think that this change is masking the true bug,
No. Above replaced line was definitely bogus. CONST_STRING is a macro
that takes *one* string constant and it has likely to be own it's own
line.
See also Makefile:
# constant string support
.c.str :
$(PERL) tools/build/c2str.pl $< > $@
and the generated .str files. Improvements to that utility are welcome
to prevent such errors.
leo
Ah. Thanks for the explanation.
One literal string constant?
find . -type f | grep -v text-base | xargs grep CONST_STR | grep -v '")'
finds
./src/library.c: *prefix_str = CONST_STRING(interpreter, pwd);
which smells fishy:
if (!VTABLE_elements(interpreter, config_hash)) {
const char *pwd = ".";
char *ret;
if (prefix_str) {
*prefix_str = CONST_STRING(interpreter, pwd);
return NULL;
}
ret = mem_sys_allocate(3);
strcpy(ret, pwd);
return ret;
}
> and the generated .str files. Improvements to that utility are welcome
> to prevent such errors.
If it's supposed to only ever be a literal "" string constant, Chip suggested
a neat trick that we're now using in Perl 5 - in the macro get the C
pre-processor to append "" to the argument. The only thing that that's valid
for is a literal string.
Nicholas Clark
> On Tue, Apr 11, 2006 at 06:15:32PM +0200, Leopold Toetsch wrote:
>> No. Above replaced line was definitely bogus. CONST_STRING is a macro
>> that takes *one* string constant and it has likely to be own it's own
>> line.
>
> Ah. Thanks for the explanation.
>
> One literal string constant?
Yup.
> find . -type f | grep -v text-base | xargs grep CONST_STR | grep -v
> '")'
> finds
> ./src/library.c: *prefix_str = CONST_STRING(interpreter,
> pwd);
>
> which smells fishy:
Yeah. It's rather easy to forget, what CONST_STRING does or mix it with
const_string.
> If it's supposed to only ever be a literal "" string constant, Chip
> suggested
> a neat trick that we're now using in Perl 5 - in the macro get the C
> pre-processor to append "" to the argument. The only thing that that's
> valid
> for is a literal string.
Great. Easy. And indeed it should catch all wrong usage.
> Nicholas Clark
Thanks,
leo
> ./src/library.c: *prefix_str = CONST_STRING(interpreter, pwd);
I've fixed that one now and the code in c2str.pl verifies that it gets
exactly two quote chars.
> Nicholas Clark
leo