Re: Partial linking of .lo objects, rev 2 (CVS HEAD)

[Marc Alff]

Hi Ralf

Thanks for your analysis, it opens my eyes on a lot of things I did not
even knew should be considered.

This email is a quick reply only,
I will post the modified code with more elaborate comments when it's
available (I don't dare say ready, only ready for more eyes).


Ralf Wildenhues wrote:
> Hi Marc,
>
> Sorry for the delay.  I have actually started to improve upon your patch
> myself, but then got sidetracked; so maybe it's better to present my
> current analysis to you, to allow you to pick it up from here.
>
>   

> * Marc Alff wrote on Sun, Apr 09, 2006 at 06:10:35PM CEST:
>   
>> Trying again with reloadables ...
>>
>> This patch (still proposed) is ...
>> - based on CVS HEAD,
>>     
>
> Thanks.
>
>   
>> - takes into account all the comments to date,
>> - defines functional tests (compile, link, inspect the resulting binary)
>> - uses Autotest
>>     
>
> Good.  It's probably ok to merge all the tests into one file, and
> simplify them somewhat.
>   

All the tests in one file may be a bit extreme, especially considering
that some tests need to be run or skipped depending
on the platform (static and shared might not be always supported), but
some merging and simplification will be better.
I will look into that.

> The whole patch looks much better already.  Comments:
> - the patch can be simplified to use func_write_libtool_object now.
> - $libobj needs to be removed before linking, else an old leftover
>   may confuse `make' in an error case.
> - for the same reason, $libobj needs to be created atomically
>   (done with `mv') (this is done by using func_write_libtool_object).
>   

Thanks for creating func_write_libtool_object to support this change (it
did not exist when the patch was written,
and was created as a result of it as I understand). Will use it from now.

> - LDFLAGS may not be overridden in the test suite, only appended to
>   (some vital flag may have to remain in there, like -m64)
>   
Good point. It's one of the things that look so evident once said, and
that I was totally unaware of.

> - We may need to rethink whether LDFLAGS or the contents of a different
>   variable should be passed to the reload line;
>   There are traps to avoid here in several directions; I haven't thought
>   through this completely; probably necessary to just filter out a few
>   flags we can allow through, like -64 or so.
>   
Will investigate.
In the mine field area, there is also something I had not considered
previously, that need some thinking :
I don't know yet how the patch and the test cases behaves in case of
cross compiling (well, cross linking to be exact).

> - .libs needs to be replaced with $objdir in the tests
>   
Ok.
> - I believe you cannot assume `strings' to be present (unsure about
>   this), so it would be prudent to test this.
> - `! test' is not portable, but `test !' is.
>   

Short of reading the POSIX spec to find what is portable in theory, and
the bug reports all other the Internet
to find what is portable in practice, can you or anyone recommend :
- a good shell scripting cookbook somewhere (practical, not 200 pages
dedicated to one specific version of a shell anyway),
- some tools to inspect the code for portability issues for shell
scripts (like autoscan for C/C++ code),
- a web museum of coding horrors and war stories on broken platforms
(a.k.a, how to not code and why)

Would be greatly appreciated.

> - Please change `cat file | grep ..'  ->  `grep .. file'
>   
Doh, did I wrote this silly code ? I guess should cut the coffee and get
more sleep ...

> - Please use the GNU CodingStyle indenting (a space before '(', and no
>   spaces before ';', ')', etc.  I don't like the GCS much either, but
>   I dislike a wild mixture of styles even more.
>
>   
Agreed that mixing styles is a bad thing (TM), so will avoid the greater
evil.

> Corner cases:
> - some objects have been compiled PIC-only, then the corresponding
>   objects will be missing in the output.  I think this is a bug in the
>   current patch.
>
>   
Don't know in detail what this refers to, will have to get back to you
on that.

> In the test suite parts you posted, there is a lot of duplication; that
> is difficult to maintain cleanly.  Luckily, there are ways out of this:
> use m4 macros to define common parts, and merge similar tests into one,
> by using shell loops and such.  (Look at some other tests for examples,
> convenience.at or static.at; some useful macros are in testsuite.at.)
>
>   
The good part about the duplication is that the tests are really, really
dumb, and self contained, so this was actually on purpose.
This decreases the chance of a bug in the test code itself, maybe due to
more complex m4 macros expanding
the wrong way without noticing it, that could mask a real bug in the
production code.

I will look into other .at files for inspiration and do some
consolidation when practical,
only as long as the test complexity does not suffer.

> I don't like the testing with "strings" for another reason: that's not
> what you would do with the output in real projects; instead, you would
> link the output into some program, or library.  So that is what the
> tests should do, verifying that the needed symbols are present.  

Using strings(1) allows to inspect more closely the binary itself.
Since, after all, the result is a binary object, looking at the object
itself seemed like a good idea to me.

In the same line of thought, I have considered using file(1) and
magic(4) to make sure that the "binary"
object is what it's expected to be.

However, this type of tests (not functional but more technical, making
strong assumptions on the implementation)
are by definition a double edged sword :
- they dive deeper to verify the expected result,
- they are more easily broken by a change of implementation (or platform
gory details) that otherwise does not change the functionality.

Since it's unclear if strings(1) is portable, I guess file(1) and
magic(4) are even less likely to be available (I won't name the usual
suspect),
so I will have to give up on white box tests, which is a shame (in my
opinion).

Back to black box (purely functional) then ...

> What's
> more, if you're testing for non-presence, the symbol names should be
> sufficiently unusual that they won't appear for unrelated reasons.
> (Some systems create quite a number of automatic symbols in object
> files).
>
>   
Never thought about possible name collisions, which can actually be
worse with grep finding non exact matches.
Thank you.


> Some more random comments:
>
> | # This test requires :
> | # - "enable static libraries"
> | 
> | AT_CHECK(
> |    [($LIBTOOL --features | grep "enable static libraries" ) || exit 77],
> |    [0],
> |    [ignore],
> |    [ignore])
>
> You could just have one test that does --tag=disable-shared; since the
> user may want to do this, it's exactly what the test should try.
>   
I don't understand, but will look into that in more details.


> FWIW, I'd prefer a bit more vertically-compact code, like this:
>
> AT_CHECK([$LIBTOOL --features | grep "enable static libraries" || exit 77],
>          [0], [ignore], [ignore])
>
>   
Simple enough
>> The code change in ltmain.m4sh is localized to the <reload_cmds> area,
>> but is more extensive, so the diff will probably be real hard to read.
>>     
>
> No, that's fine.
>
>   
>> All the parts that needs special review are commented with the keyword 
>> "REVIEW-ME", to facilitate inspection.
>>     
>
> That's not really necessary; it doesn't matter much, but by virtue of
> using "diff" it's already clear which parts have changed.  (IOW, most
> of the time I'd read the patch, only when context is necessary I'd read
> the resulting code.)
>
>   

I will stick to raw patch files then.
Beside, it will avoid the need to cleanup the patch itself once it's in
a good shape (if ever), making it safer to apply.

>> +#ifdef PIC
>> +   "PIC defined"
>> +#else
>> +   "PIC not defined"
>> +#endif
>>     
>
> Not all systems have -DPIC; you can find this out by inspecting
>  libtool --features | grep "^pic_flag="
>
>   
Thanks.

> Oh, tests of C++ functionality (including templates, and constructors),
> would be great, but are of course completely optional.  If you look at
> this, be sure to look at those bits of template support that CVS Libtool
> has, including tests/template.at.
>
>   
This would be also interesting regarding the "ld -Ur" mystical option,
to see what this thing really is about.
Time permitting.

> Cheers,
> Ralf
>
>   

All right, let's be honest. More work is needed (granted, more that I
anticipated),
which I can now start based on this round of comments.
Let's also not forget why this patch is attempted,
both for the testing side and for the libtool code itself (with regards
to relocatables).

Thanks for sharing your analysis, as you have given the patch a lot of
attention and probably time.
I should be able to "pick it up" and make something better (I hope),
while trying to limit the "cat foo | " and other shell atrocities to a
minimum.

Cheers,
Marc.