On 2009-08-25 00:13, Stephen Gran wrote:
> On Mon, Aug 24, 2009 at 01:06:10PM +0300, Török Edwin said:
>
>> On 2009-08-23 03:24, Stephen Gran wrote:
>>
>>> Hi all,
>>>
>>> This patch makes the shared/ subdirectory build a convenience library.
>>>
>> Hi Stephen,
>>
>> Did you git add shared/Makefile.am? I don't see it in your patch.
>>
>
> Arg, I must not have. Sorry about that. It boiled down to a very
> simplistic:
>
> noinst_LTLIBRARIES = libshared.la
> libshared_la_SOURCES = <bunch of source files>
> LIBS = $(top_sourcedir)/libclamav/libclamav.la
>
Ok.
>
>>> This means that on builds, each object file is only built once, then
>>> compiled into a static, partially linked object file. This has obvious
>>> advantages for the build time, as you only build these files once. It
>>> also has advantages for the maintenance of various Makefile.am's, since
>>> you just include the .la file and let libtool do the rest.
>>>
>>> Including inline since I think the list strips attachments.shar
>>>
>> There are some ifdefs in shared/, CL_NOLIBCLAMAV, and CL_NOTHREADS, with
>> your patch
>> all the code would be built as if those would not be defined.
>>
>
> Right. I think what was underlying my rather simple minded approach was
> a general feeling that some of the things being done for linker
> optimization in clamav might be unnecessary.
>
> The c library on all the POSIX platforms I'm familiar with provide stub
> functions for the pthread functions that are weak symbols and can be
> masked out by a thread library when linked to one, but can be effectively
> no-ops otherwise. Consider this simple bit of c:
>
> #include <pthread.h>
> #include <stdio.h>
> #include <stdlib.h>
>
> int main (void) {
> int x = 0;
> pthread_mutex_t mut = PTHREAD_MUTEX_INITIALIZER;
> pthread_mutex_lock(&mut);
> x++;
> pthread_mutex_unlock(&mut);
> printf("%d\n", x);
> exit(0);
> }
>
> It compiles, links and runs whether or not I add a thread library to
> the link line. Are there POSIX platforms that this doesn't work on?
>
AIX for example:
$ gcc x.c
ld: 0711-317 ERROR: Undefined symbol: .pthread_mutex_lock
ld: 0711-317 ERROR: Undefined symbol: .pthread_mutex_unlock
ld: 0711-345 Use the -bloadmap or -bnoquiet option to obtain more
information.
collect2: ld returned 8 exit status
I wouldn't use the pthread symbols if I don't use -pthread or -lpthread.
Perhaps those programs that don't need pthreads (clamscan, freshclam) could
either link to pthread, or link a special .c file in shared.c that
defines pthread_mutex_lock/unlock
as dummy noop functions.
> If (as I hope and suspect) not, can't the same principle be applied in
> clamav - let the link line of the application being compiled determine
> whether these pthread symbols are relocated to the thread library or
> left as stubs in the c library? You don't need to link -lshared to
> -lpthread (or -lthread, or ....) - just link clamd to the thread library.
> As you're effectively objcopying in symbols from libshared.a at that
> point, the linker should deal with it correctly. No guarantees, of
> course, I realize there are plenty of broken platforms out there.
>
For clamd this is not a problem, .a libraries don't need to be linked
with pthread.
It is rather something for clamscan and freshclam.
>
>> NOLIBCLAMAV is needed by clamav-milter, clamdscan, and clamdtop. I
>> t is particularly important for clamdscan, we don't want clamdscan to
>> link libclamav!
>>
>
> This is another one I'm not sure I understand the importance of. If
> there are no symbols being used from libclamav, the relocation overhead
> at startup is pretty minimal.
The milter talks to clamd, if its linked to libclamav then it might need
to be upgraded everytime
libclamav is upgraded with a different soname. That seems unnecessary.
Also in the past clamdscan was using some string function from
libclamav, so it didn't work without libclamav.
I thought it'd also be nicer for packagers since they can put clamdscan
in a package that doesn't depend on libclamav.
> I agree you may be able to shave a few
> milliseconds off of startup time by not having a .NEEDED entry for
> libclamav at all, but it hardly seems worth the effort.
Its already been done, so why undo an improvement? :)
You could have a shared_nolibclamav that contains the .c files that
don't link to libclamav, and
a shared_libclamav that contains the .c files that do link to it.
Then clamd will use both .la files, clamscan&freshclam will only use one.
> I may be
> missing something obvious though, so feel free to correct me.
>
libclamav also pulls in libz, libdl, libbz2, libpthread.
clamdscan uses 19M VIRT, vs 10M VIRT currently, vs 6M VIRT if I further
reduce the dependencies.
>
>> I'm not particularly fond of the way NOLIBCLAMAV is handled, maybe it
>> would be better to split mis.c into two files, and have
>> 2 shared.la files built: shared_nolibclamav.la, shared_libclamav.la, and
>> have clamdscan link only shared_nolibclamav.la.
>>
>> Not sure what should be done about NOTHREADS, but I think currently
>> these permutations are needed:
>> shared_nolibclamav
>> shared_libclamav
>> shared_threads_libclamav
>> shared_threads_nolibclamav
>>
>> Not all files in shared/ use those defines, so you may avoid rebuilding
>> each with different CFLAGS.
>>
>
> That way seems to lie combinatorial madness. The current solution is
> far better than that :)
>
I think nothreads is not that important, it can be worked around as
suggested above (link a .c file that defines dummy functions).
Best regards,
--Edwin
_______________________________________________
http://lurker.clamav.net/list/clamav-devel.html
Please submit your patches to our Bugzilla: http://bugs.clamav.net
