Jiri's Changes

[Paul S. Person]

First of all, it is great having Jiri look at the wgml code.
Particularly my earliest efforts, which were done when I was still
thinking in terms of C++ rather than C.

But there is a problem: character-mode devices will not load, and I am
finding the macros hard to equate to the former code (which worked for
a long time). Also, I really should be working on boxing.

I have found the first problem, and it appears to be Jiri's.

Device loading is fundamental to wgml. If it does not work, nothing
else does either. Please test any future changes to the copxxx files
with /both/ PS and at least one character-mode device (TASA is what I
usually use, although in this case WHELP might be better).

Note that, while the PS driver (and output) is /far/ more interesting
than that of WHELP, it is the output of WHELP that is used by whlpcvt
and the various help compilers to produce the help files.

Also note that, while tedious. it is not hard to spot problems using
wdw. Follow these steps to begin with:

1) Start wdw for wgml plus any text file and any character mode
device.

2) Find copdev in the Modules list and select it.

3) Find "parse_device" and set a breakpoint on the first fread().

4) Execute the program; it will stop at the breakpoint.

5) Apply "Inspect" to "out_device". Use "Show" and "Pointer Memory" to
get the object in memory. Expand the window considerably.

6) The "CC" bytes are uninitialized data. Most of them should be
overwritten, although in some cases they may not be because of
packing. Thus, a one-byte value inside the cop_device struct may
actually occupy four bytes, the last three of which will remain "CC"
to the end.

7) Execute the single fread() line. Note that (for TASA) "tasadrv" is
now visible. If you count the bytes from the start of the struct to
but not including the "w", you will find 120 of them -- the size of
cop_device and the initial value of next_offset.

8) This marks the start of the additional data area. This will be
filled up with variable data from the input file. There should /never/
be any "CC" byte inside the filled-in part of the variable data: each
data element should follow immediately after the prior element,
keeping in mind that character strings are extended by one 0x00 byte.

9) Therefore, any time you see data being added to the additional data
area which leaves "CC" bytes in front of it, you have found a problem.

The first problem I found occurs with these lines

if( length > 0 ) {
if( OUT_DEV_EXPAND_CHK( length + 1 ) ) {
out_device = resize_cop_device( out_device, length + 1 );
defaultfont_ptr = OUT_DEV_MAP_OFF(
out_device->defaultfonts.fonts );
}
defaultfont_ptr[i].font_style = OUT_DEV_CUR_OFF();

which replaced these lines:

if( out_device->allocated_size < (out_device->next_offset + length) )
{
out_device = resize_cop_device( out_device, length );
defaultfont_ptr = (default_font *) ((uint8_t *) out_device +
(size_t) out_device->defaultfonts.fonts);
}

The macro OUT_DEV_CUR_OFF() is defined as:

#define OUT_DEV_CUR_OFF() (void *)(out_device->next_offset)

so the line

defaultfont_ptr[i].font_style = OUT_DEV_CUR_OFF();

is really just a (IMHO) unnecessarily obscure version of

defaultfont_ptr[i].font_style = (void *)(out_device->next_offset);

which is /not/ the same as

defaultfont_ptr = (default_font *) ((uint8_t *) out_device +
(size_t) out_device->defaultfonts.fonts);
because, at this point

next_offset == 443

while

out_device->defaultfonts.fonts == 0x0000018B

which is

out_device->defaultfonts.fonts == 395

in decimal. The difference is the size of the default_font instances
which will populate the defaultfont_block.

You see, next_offset is being set /before/ the data is added. Using it
to set the pointer used with fread() works in some cases, but not here
and probably not elsewhere, quite possibly whenever an array is being
added (as is happening here).

Further examination will show that the font name "mono01" cannot be
found, and the device cannot be loaded because, after being placed in
the wrong position, the first for letters of "mono01" are overwritten
by a NULL pointer. But the problems begin much earlier.

Please fix this. And not just for parse_device; please check and, if
needed, fix the functions parsing driver and font files as well.

I suggest verifying that the PS device loads properly as well as TASA
or WHELP. The fact that PS currently loads and appears to be working
is, I suggest, purely an accident. The device/driver/font loading code
knows nothing of what device it is processing, but PS does differ from
the character-mode devices in that it actually /uses/ the functions
that "grow" the struct using mem_realloc() (and so ultimately
realloc()).

[Jiri Malak]

Paul,

Sorry, for broken code.
I will fix it as soon as possible.
It is my fault, I did only quick test.

Jiri

Post by Paul S. Person
First of all, it is great having Jiri look at the wgml code.
Particularly my earliest efforts, which were done when I was still
thinking in terms of C++ rather than C.
But there is a problem: character-mode devices will not load, and I am
finding the macros hard to equate to the former code (which worked for
a long time). Also, I really should be working on boxing.
I have found the first problem, and it appears to be Jiri's.
Device loading is fundamental to wgml. If it does not work, nothing
else does either. Please test any future changes to the copxxx files
with /both/ PS and at least one character-mode device (TASA is what I
usually use, although in this case WHELP might be better).
Note that, while the PS driver (and output) is /far/ more interesting
than that of WHELP, it is the output of WHELP that is used by whlpcvt
and the various help compilers to produce the help files.
Also note that, while tedious. it is not hard to spot problems using
1) Start wdw for wgml plus any text file and any character mode
device.
2) Find copdev in the Modules list and select it.
3) Find "parse_device" and set a breakpoint on the first fread().
4) Execute the program; it will stop at the breakpoint.
5) Apply "Inspect" to "out_device". Use "Show" and "Pointer Memory" to
get the object in memory. Expand the window considerably.
6) The "CC" bytes are uninitialized data. Most of them should be
overwritten, although in some cases they may not be because of
packing. Thus, a one-byte value inside the cop_device struct may
actually occupy four bytes, the last three of which will remain "CC"
to the end.
7) Execute the single fread() line. Note that (for TASA) "tasadrv" is
now visible. If you count the bytes from the start of the struct to
but not including the "w", you will find 120 of them -- the size of
cop_device and the initial value of next_offset.
8) This marks the start of the additional data area. This will be
filled up with variable data from the input file. There should /never/
be any "CC" byte inside the filled-in part of the variable data: each
data element should follow immediately after the prior element,
keeping in mind that character strings are extended by one 0x00 byte.
9) Therefore, any time you see data being added to the additional data
area which leaves "CC" bytes in front of it, you have found a problem.
The first problem I found occurs with these lines
if( length > 0 ) {
if( OUT_DEV_EXPAND_CHK( length + 1 ) ) {
out_device = resize_cop_device( out_device, length + 1 );
defaultfont_ptr = OUT_DEV_MAP_OFF(
out_device->defaultfonts.fonts );
}
defaultfont_ptr[i].font_style = OUT_DEV_CUR_OFF();
if( out_device->allocated_size < (out_device->next_offset + length) )
{
out_device = resize_cop_device( out_device, length );
defaultfont_ptr = (default_font *) ((uint8_t *) out_device +
(size_t) out_device->defaultfonts.fonts);
}
#define OUT_DEV_CUR_OFF() (void *)(out_device->next_offset)
so the line
defaultfont_ptr[i].font_style = OUT_DEV_CUR_OFF();
is really just a (IMHO) unnecessarily obscure version of
defaultfont_ptr[i].font_style = (void *)(out_device->next_offset);
which is /not/ the same as
defaultfont_ptr = (default_font *) ((uint8_t *) out_device +
(size_t) out_device->defaultfonts.fonts);
because, at this point
next_offset == 443
while
out_device->defaultfonts.fonts == 0x0000018B
which is
out_device->defaultfonts.fonts == 395
in decimal. The difference is the size of the default_font instances
which will populate the defaultfont_block.
You see, next_offset is being set /before/ the data is added. Using it
to set the pointer used with fread() works in some cases, but not here
and probably not elsewhere, quite possibly whenever an array is being
added (as is happening here).
Further examination will show that the font name "mono01" cannot be
found, and the device cannot be loaded because, after being placed in
the wrong position, the first for letters of "mono01" are overwritten
by a NULL pointer. But the problems begin much earlier.
Please fix this. And not just for parse_device; please check and, if
needed, fix the functions parsing driver and font files as well.
I suggest verifying that the PS device loads properly as well as TASA
or WHELP. The fact that PS currently loads and appears to be working
is, I suggest, purely an accident. The device/driver/font loading code
knows nothing of what device it is processing, but PS does differ from
the character-mode devices in that it actually /uses/ the functions
that "grow" the struct using mem_realloc() (and so ultimately
realloc()).

[Jiri Malak]

It is fixed now by change #37582.

Jiri

[Paul S. Person]

Post by Jiri Malak
It is fixed now by change #37582.

So it is. TASA loads again!

Thanks.