Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Remove libbcc-no-libbpf shared library, change libbcc linkage… #3225

Closed
wants to merge 1 commit into from

Conversation

olsajiri
Copy link
Contributor

I'm surprised you'd remove library that's being used.

bpftrace in fedora is using it:
-DLIBBCC_LIBRARIES:PATH=/usr/lib64/libbcc-no-libbpf.so

It's the only way to have libcc together with dynamicaly linked libbpf.so.

Please let's keep it in.

@yonghong-song
Copy link
Collaborator

[buildbot, test this please]

@yonghong-song
Copy link
Collaborator

I do see @bluca approach is to static link libbpf package libbpf.a into shared libbcc.so on the system. libbcc-no-libbpf.so already became an ABI, so agree that we need to revert it. Sorry about this.

I guess either debian can also use libbcc-no-libbpf.so or they want libbcc.so to statically link with host libbpf.a, a cmake
option can be added to control this behavior.

@yonghong-song
Copy link
Collaborator

@olsajiri looks like you already reverted commit version for libbpf submodule, could you checkout latest master, do a revert and resubmit?

@yonghong-song
Copy link
Collaborator

[buildbot, ok to test]

@bluca
Copy link
Contributor

bluca commented Jan 13, 2021

It's the only way to have libcc together with dynamicaly linked libbpf.so.

This is not true though? There's the actual option to have the real library, with the real soname, dynamically linked - CMAKE_USE_LIBBPF_PACKAGE

@bluca
Copy link
Contributor

bluca commented Jan 13, 2021

It's the only way to have libcc together with dynamicaly linked libbpf.so.

This is not true though? There's the actual option to have the real library, with the real soname, dynamically linked - CMAKE_USE_LIBBPF_PACKAGE

I mean if you have a dependency on the elf library with the wrong soname, that's fine I guess, but please add it as an option. It doesn't match the pkg-config interface, the package names, nothing.

I guess either debian can also use libbcc-no-libbpf.so or they want libbcc.so to statically link with host libbpf.a, a cmake
option can be added to control this behavior.

Mmh what we need is the real library, with the real soname, dynamically linked. If an extra option is needed that's fine.

@olsajiri
Copy link
Contributor Author

It's the only way to have libcc together with dynamicaly linked libbpf.so.

This is not true though? There's the actual option to have the real library, with the real soname, dynamically linked - CMAKE_USE_LIBBPF_PACKAGE

yes, but people expect libbcc.so without libbpf.so dependency
having libbpf statically linked inside

so we had to add libbcc-no-libbpf.so to be able to use it with libbpf.so

… instead"

This reverts commit 1cb5026.

bpftrace in fedora is using it:
  -DLIBBCC_LIBRARIES:PATH=/usr/lib64/libbcc-no-libbpf.so

It's the only way to have libcc together with dynamicaly linked libbpf.so.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
@bluca
Copy link
Contributor

bluca commented Jan 13, 2021

It's the only way to have libcc together with dynamicaly linked libbpf.so.

This is not true though? There's the actual option to have the real library, with the real soname, dynamically linked - CMAKE_USE_LIBBPF_PACKAGE

yes, but people expect libbcc.so without libbpf.so dependency
having libbpf statically linked inside

I might be missing something, but if someone wants libbcc statically linked, the option can be turned off? It's off by default in fact, one has to opt-in

so we had to add libbcc-no-libbpf.so to be able to use it with libbpf.so

This is not really usable, I'm afraid. The soname is wrong. pkg-config doesn't match. I mean it's fine as an option I guess, but there needs to be a way to do the right thing as well.

@olsajiri
Copy link
Contributor Author

@olsajiri looks like you already reverted commit version for libbpf submodule, could you checkout latest master, do a revert and resubmit?

sorry, re-pushed

@olsajiri
Copy link
Contributor Author

It's the only way to have libcc together with dynamicaly linked libbpf.so.

This is not true though? There's the actual option to have the real library, with the real soname, dynamically linked - CMAKE_USE_LIBBPF_PACKAGE

yes, but people expect libbcc.so without libbpf.so dependency
having libbpf statically linked inside

I might be missing something, but if someone wants libbcc statically linked, the option can be turned off? It's off by default in fact, one has to opt-in

so we had to add libbcc-no-libbpf.so to be able to use it with libbpf.so

This is not really usable, I'm afraid. The soname is wrong. pkg-config doesn't match. I mean it's fine as an option I guess, but there needs to be a way to do the right thing as well.

I need to check new bcc code, but at the time we were adding this, it was the only way
to have libbcc without libbpf being statically linked in it, which causes problems for programs
that dynamically linked libbpf, like bpftrace

d4b3bf0 Add libbcc-no-libbpf.so library

@bluca
Copy link
Contributor

bluca commented Jan 13, 2021

It's the only way to have libcc together with dynamicaly linked libbpf.so.

This is not true though? There's the actual option to have the real library, with the real soname, dynamically linked - CMAKE_USE_LIBBPF_PACKAGE

yes, but people expect libbcc.so without libbpf.so dependency
having libbpf statically linked inside

I might be missing something, but if someone wants libbcc statically linked, the option can be turned off? It's off by default in fact, one has to opt-in

so we had to add libbcc-no-libbpf.so to be able to use it with libbpf.so

This is not really usable, I'm afraid. The soname is wrong. pkg-config doesn't match. I mean it's fine as an option I guess, but there needs to be a way to do the right thing as well.

I need to check new bcc code, but at the time we were adding this, it was the only way
to have libbcc without libbpf being statically linked in it, which causes problems for programs
that dynamically linked libbpf, like bpftrace

d4b3bf0 Add libbcc-no-libbpf.so library

Right, the commit you are reverting makes libbcc dynamically link with libbpf when CMAKE_USE_LIBBPF_PACKAGE is used. Isn't that what you want? It's opt-in, and users who don't want that simply don't set it and get the static linking.

@yonghong-song
Copy link
Collaborator

@olsajiri could you check whether bpftrace works properly with -DLIBBCC_LIBRARIES:PATH=/usr/lib64/libbcc.so or not if CMAKE_USE_LIBBPF_PACKAGE is used to build bcc?

@olsajiri
Copy link
Contributor Author

@olsajiri could you check whether bpftrace works properly with -DLIBBCC_LIBRARIES:PATH=/usr/lib64/libbcc.so or not if CMAKE_USE_LIBBPF_PACKAGE is used to build bcc?

it probably will work, I'll try

but the point is that I was told libbcc needs to stay as is with libbpf compiled in,
so that's why we created with libbcc-no-libbpf

I think I won't be able to persuade libbcc fedora maintainer to change that,
if there's a chance we will break existing users

@bluca
Copy link
Contributor

bluca commented Jan 14, 2021

@olsajiri could you check whether bpftrace works properly with -DLIBBCC_LIBRARIES:PATH=/usr/lib64/libbcc.so or not if CMAKE_USE_LIBBPF_PACKAGE is used to build bcc?

it probably will work, I'll try

but the point is that I was told libbcc needs to stay as is with libbpf compiled in,
so that's why we created with libbcc-no-libbpf

And that will keep happening with CMAKE_USE_LIBBPF_PACKAGE=off which is the default, as mentioned, so it shouldn't be a problem? Unless I'm missing something.

Again, if you also want at the same time to keep the separate elf shared object with the broken soname that doesn't match pkg-config, I don't have any problem with that - but please add a separate option that can be disabled independently so that we can also do the right thing as well. Thanks!

@olsajiri
Copy link
Contributor Author

@olsajiri could you check whether bpftrace works properly with -DLIBBCC_LIBRARIES:PATH=/usr/lib64/libbcc.so or not if CMAKE_USE_LIBBPF_PACKAGE is used to build bcc?

it probably will work, I'll try
but the point is that I was told libbcc needs to stay as is with libbpf compiled in,
so that's why we created with libbcc-no-libbpf

And that will keep happening with CMAKE_USE_LIBBPF_PACKAGE=off which is the default, as mentioned, so it shouldn't be a problem? Unless I'm missing something.

well, we need libbcc (with libbpf compiled in) and libbcc-no-libbpf at the same time
compiled for bcc package

and that was the case until this commit:
300296a cmake: always link to packaged libbpf if CMAKE_USE_LIBBPF_PACKAGE is set (#3210)

Again, if you also want at the same time to keep the separate elf shared object with the broken soname that doesn't match pkg-config, I don't have any problem with that - but please add a separate option that can be disabled independently so that we can also do the right thing as well. Thanks!

what's wrong with the soname?

$ readelf -a /usr/lib64/libbcc.so.0 | grep SONAME
0x000000000000000e (SONAME) Library soname: [libbcc.so.0]

$ readelf -a /usr/lib64/libbcc-no-libbpf.so.0 | grep SONAME
0x000000000000000e (SONAME) Library soname: [libbcc-no-libbpf.so.0]

thanks

@bluca
Copy link
Contributor

bluca commented Jan 14, 2021

@olsajiri could you check whether bpftrace works properly with -DLIBBCC_LIBRARIES:PATH=/usr/lib64/libbcc.so or not if CMAKE_USE_LIBBPF_PACKAGE is used to build bcc?

it probably will work, I'll try
but the point is that I was told libbcc needs to stay as is with libbpf compiled in,
so that's why we created with libbcc-no-libbpf

And that will keep happening with CMAKE_USE_LIBBPF_PACKAGE=off which is the default, as mentioned, so it shouldn't be a problem? Unless I'm missing something.

well, we need libbcc (with libbpf compiled in) and libbcc-no-libbpf at the same time
compiled for bcc package

and that was the case until this commit:
300296a cmake: always link to packaged libbpf if CMAKE_USE_LIBBPF_PACKAGE is set (#3210)

Sorry, maybe I misunderstood - I thought the different cases were needed by different people in different situations? Are you saying that you need that at the exact same time for the exact same build ? What's the use case, if you don't mind me asking?

Again, if you also want at the same time to keep the separate elf shared object with the broken soname that doesn't match pkg-config, I don't have any problem with that - but please add a separate option that can be disabled independently so that we can also do the right thing as well. Thanks!

what's wrong with the soname?

$ readelf -a /usr/lib64/libbcc.so.0 | grep SONAME
0x000000000000000e (SONAME) Library soname: [libbcc.so.0]

$ readelf -a /usr/lib64/libbcc-no-libbpf.so.0 | grep SONAME
0x000000000000000e (SONAME) Library soname: [libbcc-no-libbpf.so.0]

thanks

It's mangled with the no-libbpf suffix, so it doesn't match the pkg-config metadata. It also makes it seem like it's a separate interface with a separate ABI, when in reality it's exactly the same thing, which is a bit weird.

@olsajiri
Copy link
Contributor Author

@olsajiri could you check whether bpftrace works properly with -DLIBBCC_LIBRARIES:PATH=/usr/lib64/libbcc.so or not if CMAKE_USE_LIBBPF_PACKAGE is used to build bcc?

it probably will work, I'll try
but the point is that I was told libbcc needs to stay as is with libbpf compiled in,
so that's why we created with libbcc-no-libbpf

And that will keep happening with CMAKE_USE_LIBBPF_PACKAGE=off which is the default, as mentioned, so it shouldn't be a problem? Unless I'm missing something.

well, we need libbcc (with libbpf compiled in) and libbcc-no-libbpf at the same time
compiled for bcc package
and that was the case until this commit:
300296a cmake: always link to packaged libbpf if CMAKE_USE_LIBBPF_PACKAGE is set (#3210)

Sorry, maybe I misunderstood - I thought the different cases were needed by different people in different situations? Are you saying that you need that at the exact same time for the exact same build ? What's the use case, if you don't mind me asking?

bcc package providing libraries is the use case

it provides libbcc (with libbpf compiled in)
and libbcc-no-libbpf that we use to compile bpftrace with

when we compile bpftrace with libbcc, the dynamic linker gets confused,
because of the extra compiled libbpf inside libbcc

my first fix was to have libcc dynamically linked with libbpf (like you did),
but I was told libbcc needs to stay as is because:

  1. libbpf package is not widespread
  2. libbcc is using latest libbpf from git submodule and package will not keep up
  3. peoples' apps linking libbcc will be broken

while 1) and 2) might not be a big issue anymore, I'm worried about 3)

Again, if you also want at the same time to keep the separate elf shared object with the broken soname that doesn't match pkg-config, I don't have any problem with that - but please add a separate option that can be disabled independently so that we can also do the right thing as well. Thanks!

what's wrong with the soname?
$ readelf -a /usr/lib64/libbcc.so.0 | grep SONAME
0x000000000000000e (SONAME) Library soname: [libbcc.so.0]
$ readelf -a /usr/lib64/libbcc-no-libbpf.so.0 | grep SONAME
0x000000000000000e (SONAME) Library soname: [libbcc-no-libbpf.so.0]
thanks

It's mangled with the no-libbpf suffix, so it doesn't match the pkg-config metadata. It also makes it seem like it's a separate interface with a separate ABI, when in reality it's exactly the same thing, which is a bit weird.

I guess we can just fix pkg-config metadata then?

@bluca
Copy link
Contributor

bluca commented Jan 14, 2021

@olsajiri could you check whether bpftrace works properly with -DLIBBCC_LIBRARIES:PATH=/usr/lib64/libbcc.so or not if CMAKE_USE_LIBBPF_PACKAGE is used to build bcc?

it probably will work, I'll try
but the point is that I was told libbcc needs to stay as is with libbpf compiled in,
so that's why we created with libbcc-no-libbpf

And that will keep happening with CMAKE_USE_LIBBPF_PACKAGE=off which is the default, as mentioned, so it shouldn't be a problem? Unless I'm missing something.

well, we need libbcc (with libbpf compiled in) and libbcc-no-libbpf at the same time
compiled for bcc package
and that was the case until this commit:
300296a cmake: always link to packaged libbpf if CMAKE_USE_LIBBPF_PACKAGE is set (#3210)

Sorry, maybe I misunderstood - I thought the different cases were needed by different people in different situations? Are you saying that you need that at the exact same time for the exact same build ? What's the use case, if you don't mind me asking?

bcc package providing libraries is the use case

it provides libbcc (with libbpf compiled in)
and libbcc-no-libbpf that we use to compile bpftrace with

when we compile bpftrace with libbcc, the dynamic linker gets confused,
because of the extra compiled libbpf inside libbcc

my first fix was to have libcc dynamically linked with libbpf (like you did),
but I was told libbcc needs to stay as is because:

  1. libbpf package is not widespread
  2. libbcc is using latest libbpf from git submodule and package will not keep up

Those only applies to local builds though, it doesn't apply to a distribution (where in fact it's the opposite - vendoring means out of date libbpf that doesn't track latest changes and always falls behind). The existing option allows users to do this, and it's the default so there should be no surprise nor regressions for direct users and developers, right?

  1. peoples' apps linking libbcc will be broken

while 1) and 2) might not be a big issue anymore, I'm worried about 3)

How would that happen? The breakages, as you said, come from the linker getting confused because of multiple built-in and dynamic objects. What cases are there where the opposite is true?

Again, if you also want at the same time to keep the separate elf shared object with the broken soname that doesn't match pkg-config, I don't have any problem with that - but please add a separate option that can be disabled independently so that we can also do the right thing as well. Thanks!

what's wrong with the soname?
$ readelf -a /usr/lib64/libbcc.so.0 | grep SONAME
0x000000000000000e (SONAME) Library soname: [libbcc.so.0]
$ readelf -a /usr/lib64/libbcc-no-libbpf.so.0 | grep SONAME
0x000000000000000e (SONAME) Library soname: [libbcc-no-libbpf.so.0]
thanks

It's mangled with the no-libbpf suffix, so it doesn't match the pkg-config metadata. It also makes it seem like it's a separate interface with a separate ABI, when in reality it's exactly the same thing, which is a bit weird.

I guess we can just fix pkg-config metadata then?

By all means feel free to add an entirely separate metadata file, but that doesn't work for me at all. The real thing being the real thing is required. This is just one library out of millions, it's not special in any way, and having special casing and special handling and special everything beyond a build-time config flag is just unsustainable, sorry. The build flag can be opt-in and disabled by default, that's not an issue.

@olsajiri
Copy link
Contributor Author

@olsajiri could you check whether bpftrace works properly with -DLIBBCC_LIBRARIES:PATH=/usr/lib64/libbcc.so or not if CMAKE_USE_LIBBPF_PACKAGE is used to build bcc?

it probably will work, I'll try
but the point is that I was told libbcc needs to stay as is with libbpf compiled in,
so that's why we created with libbcc-no-libbpf

And that will keep happening with CMAKE_USE_LIBBPF_PACKAGE=off which is the default, as mentioned, so it shouldn't be a problem? Unless I'm missing something.

well, we need libbcc (with libbpf compiled in) and libbcc-no-libbpf at the same time
compiled for bcc package
and that was the case until this commit:
300296a cmake: always link to packaged libbpf if CMAKE_USE_LIBBPF_PACKAGE is set (#3210)

Sorry, maybe I misunderstood - I thought the different cases were needed by different people in different situations? Are you saying that you need that at the exact same time for the exact same build ? What's the use case, if you don't mind me asking?

bcc package providing libraries is the use case
it provides libbcc (with libbpf compiled in)
and libbcc-no-libbpf that we use to compile bpftrace with
when we compile bpftrace with libbcc, the dynamic linker gets confused,
because of the extra compiled libbpf inside libbcc
my first fix was to have libcc dynamically linked with libbpf (like you did),
but I was told libbcc needs to stay as is because:

  1. libbpf package is not widespread
  2. libbcc is using latest libbpf from git submodule and package will not keep up

Those only applies to local builds though, it doesn't apply to a distribution (where in fact it's the opposite - vendoring means out of date libbpf that doesn't track latest changes and always falls behind). The existing option allows users to do this, and it's the default so there should be no surprise nor regressions for direct users and developers, right?

  1. peoples' apps linking libbcc will be broken

while 1) and 2) might not be a big issue anymore, I'm worried about 3)

How would that happen? The breakages, as you said, come from the linker getting confused because of multiple built-in and dynamic objects. What cases are there where the opposite is true?

missing libbpf dependency

but as you mentioned above that's probably not big deal for distributions,
because they provide the needed libbpf, just local builds, maybe I could
sell this to bcc maintainer, I'll ask

I tested bpftrace with libbcc and it seems to work

@bluca
Copy link
Contributor

bluca commented Jan 14, 2021

@olsajiri could you check whether bpftrace works properly with -DLIBBCC_LIBRARIES:PATH=/usr/lib64/libbcc.so or not if CMAKE_USE_LIBBPF_PACKAGE is used to build bcc?

it probably will work, I'll try
but the point is that I was told libbcc needs to stay as is with libbpf compiled in,
so that's why we created with libbcc-no-libbpf

And that will keep happening with CMAKE_USE_LIBBPF_PACKAGE=off which is the default, as mentioned, so it shouldn't be a problem? Unless I'm missing something.

well, we need libbcc (with libbpf compiled in) and libbcc-no-libbpf at the same time
compiled for bcc package
and that was the case until this commit:
300296a cmake: always link to packaged libbpf if CMAKE_USE_LIBBPF_PACKAGE is set (#3210)

Sorry, maybe I misunderstood - I thought the different cases were needed by different people in different situations? Are you saying that you need that at the exact same time for the exact same build ? What's the use case, if you don't mind me asking?

bcc package providing libraries is the use case
it provides libbcc (with libbpf compiled in)
and libbcc-no-libbpf that we use to compile bpftrace with
when we compile bpftrace with libbcc, the dynamic linker gets confused,
because of the extra compiled libbpf inside libbcc
my first fix was to have libcc dynamically linked with libbpf (like you did),
but I was told libbcc needs to stay as is because:

  1. libbpf package is not widespread
  2. libbcc is using latest libbpf from git submodule and package will not keep up

Those only applies to local builds though, it doesn't apply to a distribution (where in fact it's the opposite - vendoring means out of date libbpf that doesn't track latest changes and always falls behind). The existing option allows users to do this, and it's the default so there should be no surprise nor regressions for direct users and developers, right?

  1. peoples' apps linking libbcc will be broken

while 1) and 2) might not be a big issue anymore, I'm worried about 3)

How would that happen? The breakages, as you said, come from the linker getting confused because of multiple built-in and dynamic objects. What cases are there where the opposite is true?

missing libbpf dependency

but as you mentioned above that's probably not big deal for distributions,
because they provide the needed libbpf, just local builds, maybe I could
sell this to bcc maintainer, I'll ask

It's packaged, so the libbc package will depend on libbpf - it cannot be missing. It can only be missing if it wasn't used at build time because the option was disabled, but then it would be statically linked so no issue either.

As far as I can see, missing libbpf is only a problem if someone installs the libbpf-dev package and does a manual, local build of bcc and enables the option (disabled by default) and separately, manually ships the result somewhere else. I mean, it's possible, but there's a lot of opt-in steps that need to be voluntarily taken to get there, and one can just not opt-in...
I do not see how this is a problem for distro packages of bcc in any way though, since RPM takes care of tracking everything that needs to be tracked. I might be missing something, of course.

I tested bpftrace with libbcc and it seems to work

Nice!

@yonghong-song
Copy link
Collaborator

@olsajiri @bluca Looks the current mechanism works. I will hold this patch then.

@olsajiri
Copy link
Contributor Author

@olsajiri @bluca Looks the current mechanism works. I will hold this patch then.

yep, I'm waiting for the fedora bcc maintainer reply, I'll report back ;-)

@olsajiri
Copy link
Contributor Author

@olsajiri @bluca Looks the current mechanism works. I will hold this patch then.

yep, I'm waiting for the fedora bcc maintainer reply, I'll report back ;-)

I talked to fedora's bcc maintainer and he's fine with that,
so we can scratch this

thanks and sorry for the noise ;-)

@yonghong-song
Copy link
Collaborator

@olsajiri @bluca Looks the current mechanism works. I will hold this patch then.

yep, I'm waiting for the fedora bcc maintainer reply, I'll report back ;-)

I talked to fedora's bcc maintainer and he's fine with that,
so we can scratch this

thanks and sorry for the noise ;-)

Great. Thanks for good news :-)

@bluca
Copy link
Contributor

bluca commented Jan 21, 2021

@olsajiri @bluca Looks the current mechanism works. I will hold this patch then.

yep, I'm waiting for the fedora bcc maintainer reply, I'll report back ;-)

I talked to fedora's bcc maintainer and he's fine with that,
so we can scratch this

thanks and sorry for the noise ;-)

Great news, thanks for double checking!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants