[PATCH] pseudo: Drop static linking to sqlite3

[PATCH] pseudo: Drop static linking to sqlite3

[Richard Purdie]

Back in 2010[1] we made pseudo statically link against sqlite3. Since then
the world has changed, pseudo now has separate processes for the database
in the server and the client and they have separate linking commands.

[1] http://git.yoctoproject.org/cgit.cgi/poky/commit/?id=ad0ac0ecd38fc77daf42485489fccc10a5e1e3e7

The static sqlite3-native is causing us problems, in particular:

tmp/work/x86_64-linux/pseudo-native/1.9.0+gitAUTOINC+060058bb29-r0/recipe-sysroot-native/usr/lib/libsqlite3.a(sqlite3.o):(.data.rel+0xb0): undefined reference to `fcntl64'

which occurs if sqlite3-native was built on a machine with glibc 2.28 or later
and pseudo-native is being built on glibc before that. With dyanmical linking,
libc is backwards compatible and works but with static linking it does not.

There appears to be no easy way to avoid this other than adding a copy of
sqlite3 into the pseudo recipe. Given the static linking doesn't seem to
be required any longer due to the separate processes, drop that to fix
those issues.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 meta/conf/distro/include/no-static-libs.inc |  4 ----
 meta/recipes-devtools/pseudo/pseudo.inc     | 17 ++---------------
 2 files changed, 2 insertions(+), 19 deletions(-)

diff --git a/meta/conf/distro/include/no-static-libs.inc b/meta/conf/distro/include/no-static-libs.inc
index 4141ecb7654..a3a865cac45 100644
--- a/meta/conf/distro/include/no-static-libs.inc
+++ b/meta/conf/distro/include/no-static-libs.inc
@@ -15,10 +15,6 @@ DISABLE_STATIC_pn-nativesdk-libcap = ""
 DISABLE_STATIC_pn-libpcap = ""
 # needed by gdb
 DISABLE_STATIC_pn-readline = ""
-# needed by pseudo
-DISABLE_STATIC_pn-sqlite3 = ""
-DISABLE_STATIC_pn-sqlite3-native = ""
-DISABLE_STATIC_pn-nativesdk-sqlite3 = ""
 # openjade/sgml-common have build issues without static libs
 DISABLE_STATIC_pn-sgml-common-native = ""
 DISABLE_STATIC_pn-openjade-native = ""
diff --git a/meta/recipes-devtools/pseudo/pseudo.inc b/meta/recipes-devtools/pseudo/pseudo.inc
index 8b349097263..7ff8e449e9a 100644
--- a/meta/recipes-devtools/pseudo/pseudo.inc
+++ b/meta/recipes-devtools/pseudo/pseudo.inc
@@ -30,23 +30,10 @@ PSEUDO_EXTRA_OPTS ?= "--enable-force-async --without-passwd-fallback --enable-ep
 
 # Compile for the local machine arch...
 do_compile () {
-        SQLITE_LDADD='$(SQLITE)/$(SQLITE_LIB)/libsqlite3.a'
-	for sqlite_link_opt in $(pkg-config sqlite3 --libs --static)
-	do
-	    case "$sqlite_link_opt" in
-	    -lsqlite3)
-		;;
-	    -l*)
-		SQLITE_LDADD="${SQLITE_LDADD} ${sqlite_link_opt}"
-		;;
-	    *)
-		;;
-	    esac
-	done
 	if [ "${SITEINFO_BITS}" = "64" ]; then
-	  ${S}/configure ${PSEUDO_EXTRA_OPTS} --prefix=${prefix} --libdir=${prefix}/lib/pseudo/lib${SITEINFO_BITS} --with-sqlite-lib=${baselib} --with-sqlite=${STAGING_DIR_TARGET}${exec_prefix} --cflags="${CFLAGS}" --bits=${SITEINFO_BITS} --with-static-sqlite="$SQLITE_LDADD" --without-rpath
+	  ${S}/configure ${PSEUDO_EXTRA_OPTS} --prefix=${prefix} --libdir=${prefix}/lib/pseudo/lib${SITEINFO_BITS} --with-sqlite-lib=${baselib} --with-sqlite=${STAGING_DIR_TARGET}${exec_prefix} --cflags="${CFLAGS}" --bits=${SITEINFO_BITS} --without-rpath
 	else
-	  ${S}/configure ${PSEUDO_EXTRA_OPTS} --prefix=${prefix} --libdir=${prefix}/lib/pseudo/lib --with-sqlite-lib=${baselib} --with-sqlite=${STAGING_DIR_TARGET}${exec_prefix} --cflags="${CFLAGS}" --bits=${SITEINFO_BITS} --with-static-sqlite="$SQLITE_LDADD" --without-rpath
+	  ${S}/configure ${PSEUDO_EXTRA_OPTS} --prefix=${prefix} --libdir=${prefix}/lib/pseudo/lib --with-sqlite-lib=${baselib} --with-sqlite=${STAGING_DIR_TARGET}${exec_prefix} --cflags="${CFLAGS}" --bits=${SITEINFO_BITS} --without-rpath
 	fi
 	oe_runmake ${MAKEOPTS}
 }
-- 
2.20.1

Re: [PATCH] pseudo: Drop static linking to sqlite3

[Seebs]

On Sat,  9 Nov 2019 15:35:59 +0000
Richard Purdie <richard.purdie@linuxfoundation.org> wrote:

> Back in 2010[1] we made pseudo statically link against sqlite3. Since
> then the world has changed, pseudo now has separate processes for the
> database in the server and the client and they have separate linking
> commands.

I'm not sure what that has to do with the reasons for it being
static? Not that I remember exactly anymore, but I think we were hitting
problems caused if pseudo was dynamically linked to sqlite, but I
don't remember what they were.

I'm not entirely sure I know what you mean by "separate processes for
the database in the server and the client". I don't think libpseudo.so
should ever do any database things at all, and it shouldn't need to link
to sqlite.

> which occurs if sqlite3-native was built on a machine with glibc 2.28
> or later and pseudo-native is being built on glibc before that.

I don't think pseudo's ever been *expected* to work when built linked
against things built against different libc. It is *way* too full of
unholy magics that depend on knowing which libc it's using. That said,
the pseudo server also shouldn't really need to be statically-linked,
the only times it ever runs under pseudo, it just immediately tries to
escape and not run that way.

> There appears to be no easy way to avoid this other than adding a
> copy of sqlite3 into the pseudo recipe. Given the static linking
> doesn't seem to be required any longer due to the separate processes,
> drop that to fix those issues.

I don't understand the "separate processes" thing. So far as I know,
pseudo has never talked to sqlite from the client. I think Jason Wessel
had experimental patches at one point to do client-local sqlite work
to avoid the IPC overhead, but I don't think it ever got merged because
it turns out the IPC overhead is insignificant compared to everything
else.

-s

Re: [PATCH] pseudo: Drop static linking to sqlite3

[Richard Purdie]

On Sat, 2019-11-09 at 10:07 -0600, Seebs wrote:
> On Sat,  9 Nov 2019 15:35:59 +0000
> Richard Purdie <richard.purdie@linuxfoundation.org> wrote:
> 
> > Back in 2010[1] we made pseudo statically link against sqlite3.
> > Since then the world has changed, pseudo now has separate processes
> > for the database in the server and the client and they have
> > separate linking commands.
> 
> I'm not sure what that has to do with the reasons for it being
> static? Not that I remember exactly anymore, but I think we were
> hitting problems caused if pseudo was dynamically linked to sqlite,
> but I don't remember what they were.

I did talk briefly to Mark (also cc'd) as he wrote the original patch
and he thought it was possibly because the client was also linking
against sqlite3 and due to the other things the client does, that was
problematic.

> I'm not entirely sure I know what you mean by "separate processes for
> the database in the server and the client". I don't think
> libpseudo.so should ever do any database things at all, and it
> shouldn't need to link to sqlite.

I was under the impression the client may once have linked against it.
I admit I didn't dive into history, just checked that it clearly
doesn't now.

> > which occurs if sqlite3-native was built on a machine with glibc
> > 2.28 or later and pseudo-native is being built on glibc before
> > that.
> 
> I don't think pseudo's ever been *expected* to work when built linked
> against things built against different libc. It is *way* too full of
> unholy magics that depend on knowing which libc it's using. That
> said, the pseudo server also shouldn't really need to be statically-
> linked, the only times it ever runs under pseudo, it just immediately
> tries to escape and not run that way.

The client lib doesn't and the server side should behave just like any
other linux binary afaik so we should be ok with a dynamicly linked
sqlite3?

> > There appears to be no easy way to avoid this other than adding a
> > copy of sqlite3 into the pseudo recipe. Given the static linking
> > doesn't seem to be required any longer due to the separate
> > processes,
> > drop that to fix those issues.
> 
> I don't understand the "separate processes" thing. So far as I know,
> pseudo has never talked to sqlite from the client. I think Jason
> Wessel had experimental patches at one point to do client-local
> sqlite work to avoid the IPC overhead, but I don't think it ever got
> merged because it turns out the IPC overhead is insignificant
> compared to everything else.

If I'm working off incorrect assumptions, is there another reason we
need a static sqlite? I have tested this patch before I posted it and
it seems fine in our test builds...

Cheers,

Richard

Re: [PATCH] pseudo: Drop static linking to sqlite3

[Seebs]

On Sat, 09 Nov 2019 16:30:41 +0000
Richard Purdie <richard.purdie@linuxfoundation.org> wrote:

> I did talk briefly to Mark (also cc'd) as he wrote the original patch
> and he thought it was possibly because the client was also linking
> against sqlite3 and due to the other things the client does, that was
> problematic.

It *shouldn't* link against sqlite3. But! The commit in question refers
to RHEL5 and LD_LIBRARY_PATH, and I think that shook loose a memory:

I think at one point, we had a Crucial Bug Fix in sqlite3, in our build
system, and if we didn't statically link, there was a risk of getting
the broken version at runtime.

> The client lib doesn't and the server side should behave just like any
> other linux binary afaik so we should be ok with a dynamicly linked
> sqlite3?

Yes.

The issue here was, I believe, not "dynamically-linked sqlite3 per se",
but "dynamic linking, plus LD_LIBRARY_PATH, picking an sqlite3 which
caused us specific problems".

In the Yocto environment, I think we're reasonably sure that we always
get a clean Yocto-built sqlite3, and that *should* be fine.

So I'd say go for it, but if you see weird sqlite3 stuff that happens
only very occasionally, look at this first. :P

-s

Re: [PATCH] pseudo: Drop static linking to sqlite3

[Andre McCurdy]

On Sat, Nov 9, 2019 at 8:46 AM Seebs <seebs@seebs.net> wrote:
>
> On Sat, 09 Nov 2019 16:30:41 +0000
> Richard Purdie <richard.purdie@linuxfoundation.org> wrote:
>
> > I did talk briefly to Mark (also cc'd) as he wrote the original patch
> > and he thought it was possibly because the client was also linking
> > against sqlite3 and due to the other things the client does, that was
> > problematic.
>
> It *shouldn't* link against sqlite3. But! The commit in question refers
> to RHEL5 and LD_LIBRARY_PATH, and I think that shook loose a memory:
>
> I think at one point, we had a Crucial Bug Fix in sqlite3, in our build
> system, and if we didn't statically link, there was a risk of getting
> the broken version at runtime.
>
> > The client lib doesn't and the server side should behave just like any
> > other linux binary afaik so we should be ok with a dynamicly linked
> > sqlite3?
>
> Yes.
>
> The issue here was, I believe, not "dynamically-linked sqlite3 per se",
> but "dynamic linking, plus LD_LIBRARY_PATH, picking an sqlite3 which
> caused us specific problems".
>
> In the Yocto environment, I think we're reasonably sure that we always
> get a clean Yocto-built sqlite3, and that *should* be fine.
>
> So I'd say go for it, but if you see weird sqlite3 stuff that happens
> only very occasionally, look at this first. :P

With this merged, we can also drop the hack to force the sqlite static
lib to be PIC:

  https://git.openembedded.org/openembedded-core/commit/?id=6a58e12d19c539deac9e90679a68438497a42fa4

Re: [PATCH] pseudo: Drop static linking to sqlite3

[Khem Raj]

On Mon, 2019-11-11 at 05:04 -0800, Andre McCurdy wrote:
> On Sat, Nov 9, 2019 at 8:46 AM Seebs <seebs@seebs.net> wrote:
> > On Sat, 09 Nov 2019 16:30:41 +0000
> > Richard Purdie <richard.purdie@linuxfoundation.org> wrote:
> > 
> > > I did talk briefly to Mark (also cc'd) as he wrote the original
> > > patch
> > > and he thought it was possibly because the client was also
> > > linking
> > > against sqlite3 and due to the other things the client does, that
> > > was
> > > problematic.
> > 
> > It *shouldn't* link against sqlite3. But! The commit in question
> > refers
> > to RHEL5 and LD_LIBRARY_PATH, and I think that shook loose a
> > memory:
> > 
> > I think at one point, we had a Crucial Bug Fix in sqlite3, in our
> > build
> > system, and if we didn't statically link, there was a risk of
> > getting
> > the broken version at runtime.
> > 
> > > The client lib doesn't and the server side should behave just
> > > like any
> > > other linux binary afaik so we should be ok with a dynamicly
> > > linked
> > > sqlite3?
> > 
> > Yes.
> > 
> > The issue here was, I believe, not "dynamically-linked sqlite3 per
> > se",
> > but "dynamic linking, plus LD_LIBRARY_PATH, picking an sqlite3
> > which
> > caused us specific problems".
> > 
> > In the Yocto environment, I think we're reasonably sure that we
> > always
> > get a clean Yocto-built sqlite3, and that *should* be fine.
> > 
> > So I'd say go for it, but if you see weird sqlite3 stuff that
> > happens
> > only very occasionally, look at this first. :P
> 
> With this merged, we can also drop the hack to force the sqlite
> static
> lib to be PIC:
> 
>   
> https://git.openembedded.org/openembedded-core/commit/?id=6a58e12d19c539deac9e90679a68438497a42fa4

That would be good but a caution is that if pseudo is just one usecase
here which brought this issue forward, since we use PIE by default this
would come up elsewhere too

Re: [PATCH] pseudo: Drop static linking to sqlite3

[Adrian Bunk]

On Mon, Nov 11, 2019 at 07:59:43AM -0800, Khem Raj wrote:
> On Mon, 2019-11-11 at 05:04 -0800, Andre McCurdy wrote:
> > 
> > With this merged, we can also drop the hack to force the sqlite
> > static
> > lib to be PIC:
> > 
> >   
> > https://git.openembedded.org/openembedded-core/commit/?id=6a58e12d19c539deac9e90679a68438497a42fa4
> 
> That would be good but a caution is that if pseudo is just one usecase
> here which brought this issue forward, since we use PIE by default this
> would come up elsewhere too

The only usecase for building a static library as PIC I am aware of is 
when you want to link the static library into a shared library.

Usually linking a static library into a shared library is a bug and
the link error is preferable since you actually intended to link
with the shared library (the linker automatically uses a static
library when it can't find a shared library).

There are rare exceptions when this is done intentionally.
Linking a static libsqlite3.a into libpseudo.so was such an exception.

Whether the non-PIC code is PIE or not does not make a difference.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] pseudo: Drop static linking to sqlite3

[Khem Raj]

[-- Attachment #1: Type: text/plain, Size: 1716 bytes --]

On Mon, Nov 11, 2019 at 8:36 AM Adrian Bunk <bunk@stusta.de> wrote:

> On Mon, Nov 11, 2019 at 07:59:43AM -0800, Khem Raj wrote:
> > On Mon, 2019-11-11 at 05:04 -0800, Andre McCurdy wrote:
> > >
> > > With this merged, we can also drop the hack to force the sqlite
> > > static
> > > lib to be PIC:
> > >
> > >
> > >
> https://git.openembedded.org/openembedded-core/commit/?id=6a58e12d19c539deac9e90679a68438497a42fa4
> >
> > That would be good but a caution is that if pseudo is just one usecase
> > here which brought this issue forward, since we use PIE by default this
> > would come up elsewhere too
>
> The only usecase for building a static library as PIC I am aware of is
> when you want to link the static library into a shared library.
>
> Usually linking a static library into a shared library is a bug and
> the link error is preferable since you actually intended to link
> with the shared library (the linker automatically uses a static
> library when it can't find a shared library).
>
> There are rare exceptions when this is done intentionally.
> Linking a static libsqlite3.a into libpseudo.so was such an exception.
>
> Whether the non-PIC code is PIE or not does not make a difference.


It’s not directly related to that more to the fact that we disable static
libs by default and since this was available some other package might have
depended on it inadvertently

>
>
> cu
> Adrian
>
> --
>
>        "Is there not promise of rain?" Ling Tan asked suddenly out
>         of the darkness. There had been need of rain for many days.
>        "Only a promise," Lao Er said.
>                                        Pearl S. Buck - Dragon Seed
>
>

[-- Attachment #2: Type: text/html, Size: 2473 bytes --]

Re: [PATCH] pseudo: Drop static linking to sqlite3

[Mark Hatle]

On 11/9/19 10:30 AM, Richard Purdie wrote:
> On Sat, 2019-11-09 at 10:07 -0600, Seebs wrote:
>> On Sat,  9 Nov 2019 15:35:59 +0000
>> Richard Purdie <richard.purdie@linuxfoundation.org> wrote:
>>
>>> Back in 2010[1] we made pseudo statically link against sqlite3.
>>> Since then the world has changed, pseudo now has separate processes
>>> for the database in the server and the client and they have
>>> separate linking commands.
>>
>> I'm not sure what that has to do with the reasons for it being
>> static? Not that I remember exactly anymore, but I think we were
>> hitting problems caused if pseudo was dynamically linked to sqlite,
>> but I don't remember what they were.
> 
> I did talk briefly to Mark (also cc'd) as he wrote the original patch
> and he thought it was possibly because the client was also linking
> against sqlite3 and due to the other things the client does, that was
> problematic.

First (early) version of pseudo would fork for the server, there wasn't a
seperate executed server and we had all sorts of problems with dynamic libraries
being loaded and loaded from the correct paths (due to the LD_PRELOAD).

Moving to two process spaces, LD_PRELOAD and the pseudo executable happened
pretty early in development, but I suspect some of this is left over from that.

>> I'm not entirely sure I know what you mean by "separate processes for
>> the database in the server and the client". I don't think
>> libpseudo.so should ever do any database things at all, and it
>> shouldn't need to link to sqlite.
> 
> I was under the impression the client may once have linked against it.
> I admit I didn't dive into history, just checked that it clearly
> doesn't now.

The client had both from above, but also for some early logging that happened
before the pseudo server was executed.  Both vestiges are long fone.

>>> which occurs if sqlite3-native was built on a machine with glibc
>>> 2.28 or later and pseudo-native is being built on glibc before
>>> that.
>>
>> I don't think pseudo's ever been *expected* to work when built linked
>> against things built against different libc. It is *way* too full of
>> unholy magics that depend on knowing which libc it's using. That
>> said, the pseudo server also shouldn't really need to be statically-
>> linked, the only times it ever runs under pseudo, it just immediately
>> tries to escape and not run that way.
> 
> The client lib doesn't and the server side should behave just like any
> other linux binary afaik so we should be ok with a dynamicly linked
> sqlite3?
> 
>>> There appears to be no easy way to avoid this other than adding a
>>> copy of sqlite3 into the pseudo recipe. Given the static linking
>>> doesn't seem to be required any longer due to the separate
>>> processes,
>>> drop that to fix those issues.
>>
>> I don't understand the "separate processes" thing. So far as I know,
>> pseudo has never talked to sqlite from the client. I think Jason
>> Wessel had experimental patches at one point to do client-local
>> sqlite work to avoid the IPC overhead, but I don't think it ever got
>> merged because it turns out the IPC overhead is insignificant
>> compared to everything else.
> 
> If I'm working off incorrect assumptions, is there another reason we
> need a static sqlite? I have tested this patch before I posted it and
> it seems fine in our test builds...
> 
> Cheers,
> 
> Richard
> 

Re: [PATCH] pseudo: Drop static linking to sqlite3

[Seebs]

On Mon, 11 Nov 2019 08:17:30 -0600
Mark Hatle <mark.hatle@kernel.crashing.org> wrote:

> First (early) version of pseudo would fork for the server, there
> wasn't a seperate executed server and we had all sorts of problems
> with dynamic libraries being loaded and loaded from the correct paths
> (due to the LD_PRELOAD).
> 
> Moving to two process spaces, LD_PRELOAD and the pseudo executable
> happened pretty early in development, but I suspect some of this is
> left over from that.

To be picky:

pseudo *still does this*. If it can't find the server, it clears the
environment and respawns it. There were problems with this for the
longest time, which turned out to be because I was calling setenv to
clean the environment, and bash *overrides setenv*, so if the program
spawning the pseudo server was bash, things went wrong. It should
actually be fixed now and work pretty well.

-s