patchgrep - Patches: patch #7017, dlopen PCRE

 
 

You are not allowed to post comments on this tracker with your current authentification level.

patch #7017: dlopen PCRE

Submitted by:  Reuben Thomas <rrt>
Submitted on:  Sat 05 Dec 2009 03:10:12 PM UTC  
 
Category: NonePriority: 5 - Normal
Status: In ProgressPrivacy: Public
Assigned to: NoneOpen/Closed: Open

(Jump to the original submission Jump to the original submission)

Wed 22 Aug 2012 05:39:35 PM UTC, comment #19:

It seems there is no interest in this patch. Debian & Ubuntu, which I use, have had a similar but simpler (because GNU-specific) patch installed for some years (which I also wrote!), so I have little incentive to keep updating the patch.

As far as I can see, the latest version of the patch is pretty much fine modulo minor tweaks, except that the C code patch must now be applied to pcresearch.c, not search.c.

If anyone would like to knock the patch into shape for current grep and install it, be my guest; it would also reduce the number of patches that Debian needs by one.

Reuben Thomas <rrt>
Fri 12 Mar 2010 11:39:45 AM UTC, comment #18:

Patch updated with soname detection.

(file #19919)

Reuben Thomas <rrt>
Thu 11 Mar 2010 03:08:41 PM UTC, comment #17:

Hmm, yeah, it would've worked if pcre.h hadn't used extern. Go ahead with your ppp then!

Paolo Bonzini <bonzini>
Thu 11 Mar 2010 03:01:52 PM UTC, comment #16:

This does not work either:

search.o: In function `load_pcre':
/home/rrt/repo/grep-dlopen-pcre/src/search.c:71: undefined reference to `dl_pcre_compile'
/home/rrt/repo/grep-dlopen-pcre/src/search.c:71: undefined reference to `dl_pcre_study'
/home/rrt/repo/grep-dlopen-pcre/src/search.c:71: undefined reference to `dl_pcre_exec'
/home/rrt/repo/grep-dlopen-pcre/src/search.c:71: undefined reference to `dl_pcre_maketables'
search.o: In function `Pexecute':
/home/rrt/repo/grep-dlopen-pcre/src/search.c:812: undefined reference to `dl_pcre_exec'
search.o: In function `Pcompile':
/home/rrt/repo/grep-dlopen-pcre/src/search.c:762: undefined reference to `dl_pcre_compile'
/home/rrt/repo/grep-dlopen-pcre/src/search.c:762: undefined reference to `dl_pcre_maketables'
/home/rrt/repo/grep-dlopen-pcre/src/search.c:766: undefined reference to `dl_pcre_study'
collect2: ld returned 1 exit status

This I do understand, because now dl_pcre_exec &c. is never actually defined. I can't see how your solution is going to work without defining the variable, copying out the type...

Reuben Thomas <rrt>
Thu 11 Mar 2010 01:06:52 PM UTC, comment #15:

I think you need a tweak:

#define pcre_exec (*dl_pcre_exec)

...

dl_pcre_exec = dlsym(handle, "pcre_exec");

(It cannot be macroized, but it can be m4-ized).

Paolo Bonzini <bonzini>
Thu 11 Mar 2010 11:33:13 AM UTC, comment #14:

I'm aware that typeof is non-ISO (but it is supported by at least Metrowerks as well as GCC), hence my test in configure.ac. I propose to be happy to fail on those systems that support dlopen but don't have a compiler with typeof.

However, another reason was that I couldn't think of another way of doing it without writing all the types out again.

So, I tried your suggestion, and I get:

search.c:71: error: lvalue required as left operand of assignment
search.c:72: error: lvalue required as left operand of assignment
search.c:73: error: lvalue required as left operand of assignment
search.c:74: error: lvalue required as left operand of assignment

I confess I do not understand this error, I thought your idea would work (though unfortunately it was not susceptible to a macro, as mine is).

Reuben Thomas <rrt>
Thu 11 Mar 2010 08:49:13 AM UTC, comment #13:

Looks nice, but typeof is a GCC extension. Can you check if it works to do something like

#define pcre_exec (*pcre_exec)
#define pcre_compile (*pcre_compile)

before including pcre.h?

When support is added for gz/bz2, it would makes sense to try autogenerating the wrapper files at configure time, like

AC_CHECK_LIB_DYNAMIC([libpcre], [pcre_exec pcre_compile])

etc. This is for later of course

Paolo Bonzini <bonzini>
Thu 11 Mar 2010 12:02:49 AM UTC, comment #12:

Thanks for the outline, I'll attempt to fill in the blanks.

Reuben Thomas <rrt>
Wed 10 Mar 2010 11:50:45 PM UTC, comment #11:

I have updated the patch to the one I meant to post, sorry about that.

Reuben Thomas <rrt>
Wed 10 Mar 2010 11:46:03 PM UTC, comment #10:

yes, that configure.ac check looks correct, but that is certainly not in any of the posted patches so far.

ldd is a host test, so that wouldnt work (i.e. it implicitly invokes the host ldso to find things out). you could use `readelf -d` and assume ELF format, but that seems like it'll break on non-ELF systems (OS X comes to mind).

a --with-pcre-soname=SONAME option would push the issue onto distro packagers who would probably be annoyed as then they'll have to implement the things i'm talking about, but would provide a way of grep washing its hand of possibly buggy code.

so if you wanted, you could do something like:
AC_ARG_WITH([pcre-soname], [...], with_pcre_soname=$withval, [
AS_IF([test "$ac_cv_lib_dl_dlopen" = "yes"], [
<link simple app against -lpcre>
with_pcre_soname=`readelf -d <simple app> 2>/dev/null | grep NEEDED.*libpcre | awk '{print $NF}'
if test -z "$with_pcre_soname" ; then
AC_MSG_ERROR([unable to detect libpcre SONAME; please use --with-pcre-soname])
fi
])])

this would satisfy most distro people while providing a fallback for the non-ELF freaks

Mike Frysinger <vapier>
Wed 10 Mar 2010 11:37:52 PM UTC, comment #9:

So, I seem to be able to find the soname at least on an ELF system as follows:

$ objdump -p /usr/lib/libpcre.so | grep SONAME
SONAME libpcre.so.3

So that would be something I could run at compile time to get the soname for runtime for a given system. Is that good enough?

Reuben Thomas <rrt>
Wed 10 Mar 2010 11:33:22 PM UTC, comment #8:

I believe the configure.ac test is correct. Here is the code:

# support for dlopen
AC_ARG_WITH(dl,
AC_HELP_STRING([--without-dl], [Don't try to use libdl]))
if test "$with_dl" != "no"; then
AC_CHECK_LIB([dl], [dlopen])
AC_C_TYPEOF
fi
AM_CONDITIONAL(USING_LIBDL, [test x$ac_cv_lib_dl_dlopen = xyes])

Here, USING_LIBDL is only set if ac_cv_lib_dl_dlopen is set to yes, which itself can only happen if --without-dl is not passed to configure AND libdl is found. Is there something wrong with this logic?

As for dlopening the library, is there a simple way to find out which library is picked up by a link? I suppose I can compile a test, link against the file, then ldd it to get the SONAME?

I am tempted, in any case, to do something simpler: dlopen("libpcre.so"), but allow the soname to be specified by a configure-time parameter.

The two stools I am trying to balance between here are on the one hand you (reasonably) want me to provide something that works on any system, while Paolo says I can't use libltdl (which would otherwise simplify matters, I think). I want to avoid reimplementing any significant functionality which I would otherwise get from libtool, libltdl, or similar.

The reason I think my suggestion is reasonable is that there are, I think, two main classes of users I'm trying to cater to:

1. Those who build grep on their own systems. They will have the development sources of PCRE installed, so a default of libpcre.so will work. Those on systems with different naming conventions can give a --with-pcre-soname= argument.

2. Those who are packaging grep. They won't mind giving the --configure argument.

Of course, if there's a really simple way portably to obtain the correct soname, I'm all for it.

Reuben Thomas <rrt>
Wed 10 Mar 2010 11:24:35 PM UTC, comment #7:

did you post the wrong file ? the latest patch still doesnt check the result of AC_CHECK_LIB(), and it's dlopening libpcre.so.

hardcoding any filename isnt going to work. the SONAME you should be hardcoding is whatever matches the pcre.h you're including. on your system it may be libpcre.so.3, but it isnt on mine. your best bet is to attempt a configure link test against -lpcre and find out what SONAME it picked up.

an alternative is to do a compile test to figure out what API the pcre.h is providing (PCRE_MAJOR/PCRE_MINOR), and maintain a small table in the configure script. for example, 7.9 provides "libpcre.so.0".

i know it sucks, but this is the reality of trying to implement this sort of dynamic API.

Mike Frysinger <vapier>
Wed 10 Mar 2010 09:51:07 PM UTC, comment #6:

I have fixed configure.ac to check the actual result of the test.

For dlopening libpcre, I have changed the name to libpcre.so.3. This is better, because it ensures the correct version of the API, and works when the development sources are absent.

What else is needed to make this patch fly?

(file #19905)

Reuben Thomas <rrt>
Wed 10 Mar 2010 03:30:28 PM UTC, comment #5:

a configure test wouldnt be useful either. people building packages will have libpcre.so on their system so the configure test would pass. but the systems where that package gets deployed will fail most of the time because people arent installing the libpcre development files. plus, libpcre.so could be a linker script and not an actual ELF.

so again, this proposed code isnt going to work.

also, the current configure code does a libdl check but then doesnt actually review the result. using_dl should not be set to yes if AC_CHECK_LIB([dl], [dlopen]) fails.

Mike Frysinger <vapier>
Wed 10 Mar 2010 03:24:01 PM UTC, comment #4:

Attached, an updated version of the patch that uses dlopen if available and desired, and cleans up the code somewhat, in
particular making the use of dlopen simpler, and adding support for all cases to configure.ac and src/Makefile.am.

(file #19900)

Reuben Thomas <rrt>
Mon 08 Mar 2010 05:20:40 PM UTC, comment #3:

This would be acceptable together with a configure test that dlopen("libpcre.so"). I don't think we want something as complicated as libltdl for grep.

Paolo Bonzini <bonzini>
Sat 05 Dec 2009 10:15:33 PM UTC, comment #2:

As I said above, I need to know which dynamic loading mechanism to use first.

Reuben Thomas <rrt>
Sat 05 Dec 2009 10:12:48 PM UTC, comment #1:

you cant dlopen(libpcre.so). you need to figure out the SONAME you're currently linking against and load that.

Mike Frysinger <vapier>
Sat 05 Dec 2009 03:10:12 PM UTC, original submission:

This patch dlopens libpcre, allowing PCRE support on systems with grep in /bin and libpcre in /usr/lib. Patch is against current git head.

Still to be finalised: the exact dlopening mechanism preferred. Currently uses dlopen directly.

Reuben Thomas <rrt>

 

Attached Files
file #19205:  grep-dlopen-pcre.diff added by rrt (2kB - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by bonzini (Posted a comment)
  • -unavailable- added by vapier (Posted a comment)
  • -unavailable- added by rrt (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 6 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 12 Mar 2010 11:39:45 AM UTCrrtAttached File-=>Added 0001-Optionally-dlopen-PCRE.patch, #19919
    Wed 10 Mar 2010 11:49:48 PM UTCrrtAttached File-=>Added 0001-Optionally-dlopen-PCRE.patch, #19906
    Wed 10 Mar 2010 09:51:07 PM UTCrrtAttached File-=>Added 0001-Optionally-dlopen-PCRE.patch, #19905
    Wed 10 Mar 2010 03:24:01 PM UTCrrtAttached File-=>Added 0001-Optionally-dlopen-PCRE.patch, #19900
    Mon 08 Mar 2010 05:20:40 PM UTCbonziniStatusNone=>In Progress
    Sat 05 Dec 2009 03:10:12 PM UTCrrtAttached File-=>Added grep-dlopen-pcre.diff, #19205

    Back to the top


    Powered by Savane 3.1-cleanup