Re: [bug #41758] VMS Make incorrectly reports archives support present.

[John E. Malmberg]

On 5/31/2014 7:21 AM, h.becker wrote:
> On 05/15/2014 05:51 AM, John Malmberg wrote:
> > Follow-up Comment #4, bug #41758 (project make):
>
> I admit I never looked at all the archive/library code before. So I may
> need some education. The patch works for my simple test but that worked
> with the unpatched version as well. So what's really different? Or which
> test cases are addressed?

This fixes 8 failing tests in the features/archives test.  Wildcard 
access now works correctly.  In addition the vpath3 test that was 
failing before now works correctly.  It needs to be modified to expect 
the result in VMS syntax.

> However, the patch doesn't work if I dare to specify a module name
> different from the file name. That worked with the unpatched version.

I need to look at that to see how it could have worked, since GNU Make 
is what is assuming that the module name is the same as the filename, 
and I do not see a syntax to tell it otherwise.

> On VAX/VMS 7.3, the version I run in simh on my laptop (when I have no
> access to the net) I get:
> %SYSTEM-F-ACCVIO, access violation, reason mask=00, virtual
> address=00000000, PC=00000000, PSL=03C00000

That has been traced down to a bug in lbr$_set_module on VAX/VMS 7.3. 
It was ignoring the size set in the input buffer and always writing 49 
bytes.

In addition both the original and my modified code was incorrectly 
passing the address of the buffer descriptor length as the variable to 
get the actual length of the header.  I did not experiment to see if the 
stack corruption went away when I fixed that.

The mhddef structure, which has the mhd$l_dattim member is sufficient.
If there is more data, the lbr$_set_module is supposed to truncate the 
data to the size given and return a status of LBR-W-HDRTRUNC which is 
2525184.  I can not find that value in any header file.

That is what was corrupting the stack on VAX.

Part of the fix is to allocate LBR$C_MAXHDRSIZ for the mhd structure and 
then free it after it is used.

Diagnosing this also exposed a bug in mhddef.h.  The definition for 
mhd$l_datim is wrong.  It has it as a 32 bit longword instead of a 64 
bit structure or array.

If the mhddef.h was correct, then the decc$fix_time() would not have 
returned anything resembling a sane time.

Also if you run the test program with no input, a null pointer get 
referenced, so that also needs a check.

Not enough time to produce a diff tonight, but thought you would like to 
know why it was failing.

> Anyway, some comments ...
>
> Having descriptive names for some variables and using const is good. But
> when introducing different mechanisms/algorithms such changes
> make it more difficult to see what's really new. Maybe having two
> patches would help, one for the code improvements and one for the "real"
> changes.

Part of the problem was that some of the old names and comments were 
completely wrong, which would make comparing the new code to the old 
code difficult at best.

> There are some new conditionals for VMS in the source code. I understand
> that they are required to have VMS specific code. But it would make the
> source more readable if such code could be moved into VMS specific
> sources or sections within the source file. I didn't try to do that nor
> do I have a real plan, it is just an idea to make this code easier to
> understand.

Unfortunately it was about the only way to do things the way the code is 
structured.  Only the caller and the callback routine know what the 
structure of the argument is, and that structure needs some VMS specific 
data in it at those points.

I can look too see if macros can be added to hide the #ifdef VMS code to 
see if that could make it easier to read.

> It seems that the existing and new code for VMS is complex, at least
> callbacks are not straight forward and deserve some good comments.
> Rather than commenting on the lines, like "On VMS, there is probably a
> saved suffix..." I suggest to have one big comment to explain the whole
> picture, examples included. Then, someone looking at the VMS specific
> code doesn't need to look at all the comments in all the conditional
> code to understand the overall picture. Also, someone who isn't
> interested in VMS code will have shorter source code sections to skip
> (OK, I should have known that everybody is using Eclipse and has folding
> for preprocessor branches enabled :-)

I will look at that tomorrow.  It is late here.

It may be that a VMS programmers notes section of the readme is needed.

> I suggest to remove the (old) comments on decc$fix_time, it looks like
> decc$fix_time is always needed (and not only on VAX and Alpha). And the
> old comments are in git anyway.

I will look at simplifying it.

> For the tests, the perl scripts, I suggest to change $vms, which is
> explained as '# $vms is for VMS with out a GNV Bash shell'. Then this is
> very likely DCL mode, or? So it should be named either DCL[_mode], or
> for GNV and bash there should be a flag named GNV[_mode|_bash]. I prefer
> the latter just because DCL is default for VMS and GNV bash is special.

I can look at that.  There are a lot of scripts that need it.

Also the perl scripts are not counting the test successes correctly on 
non-unix plaforms.  I have not investigated how to fix that.

The perl script should report:  Failed, Passed, Skipped, Expected 
Failed, and Unexpected Passed.

Expected failed and Unsexpected passed are for known bugs.  Instead of 
just skipping a test that can not currently pass on platform, it should 
be marked expected fail.  Then if someone fixes it, either on purpose or 
by accident, it will show up in the test run.

> The test for imagelib seems to work on VAX only: I only see a UNIVERSAL
> option and not a SYMBOL_VECTOR option.

I currently can only run the Perl test script on the Alpha system.  The 
symbol_vectors are not needed to test creating the library.

> For utouch in the perl scripts there is a comment saying "utouch is not
> changing what VMS library compare is testing for". I'm not sure I
> understand what the "VMS library compare" is in this context. I would
> like to have the comment saying what utouch is changing and what is
> compared. I assume it is the VMS modfication aka revision time, which is
> compared. For what it is worth, I do have a utouch.c [.exe] which
> changes the VMS modfication time by an offset - the same interface as
> the perl subroutine.

I have not traced down why the perl touch method is not working on VMS
object files used for the librarian tests.  All I know right now is that 
it does not work for me.  Since I have a workaround, I am not 
investigating it.

I can try to make the comments more clear about that.

Regards,
-John