Re: [svn:parrot] r12129 - trunk/src
Nicholas Clark
> Author: bernhard
> Date: Thu Apr 6 11:23:31 2006
> New Revision: 12129
>
> Modified:
> trunk/src/spf_render.c
>
> Log:
> Make some string formating test failures go away under
> Linux on i686.
> However I have no what had caused the failures.
>
>
> Modified: trunk/src/spf_render.c
> ==============================================================================
> --- trunk/src/spf_render.c (original)
> +++ trunk/src/spf_render.c Thu Apr 6 11:23:31 2006
> @@ -163,7 +163,14 @@
> }
>
> if ((info->flags & FLAG_WIDTH) && info->width > len) {
> - STRING *fill = CONST_STRING(interpreter, info->flags & FLAG_ZERO ? "0" : " ");
> + STRING *fill;
> +
> + if (info->flags & FLAG_ZERO) {
> + fill = CONST_STRING(interpreter, "0");
> + }
> + else {
> + fill = CONST_STRING(interpreter, " ");
> + }
>
> fill = string_repeat(interpreter, fill, info->width - len, NULL);
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
Leopold Toetsch
On Apr 7, 2006, at 19:38, Nicholas Clark wrote:
>> - 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
Nicholas Clark
>
> On Apr 7, 2006, at 19:38, Nicholas Clark wrote:
>
> >>- 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.
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
Leopold Toetsch
On Apr 11, 2006, at 22:48, Nicholas Clark wrote:
> 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
Leopold Toetsch
> ./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