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

Always build libbpf as a PIE #172

Merged
merged 1 commit into from
Mar 5, 2024
Merged

Conversation

jordalgo
Copy link
Contributor

@jordalgo jordalgo commented Mar 5, 2024

This is to fix an error sometimes seen when compiling with gcc,
whereby the position independent sched ext rust libraries
don't play nicely with the libbpf library if it's not also built
as position independent.

This also explicitly sets the rustc relocation-mode to "pic",
which is the default (just so this doesn't accidentally change
out from under us).

Comment on lines 22 to 24
if [ "$cc" = "cc" ]; then
cflags+="-fPIE "
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not understanding what the meaning is of -fPIE for a static library. My guess is that this implies -fPIC or something? Anyways, I think if we're going to do this we should just always build with -fPIE. Clearly it's something that rustc requires.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reads to me it's position independent but can't be linked to create dynamic libs. No idea why it's needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I've just never seen -fPIE used to create a lib. I thought it was always -fPIC given that it's not creating an executable.


for arg in ${args[@]:(idx+1)}; do
case $arg in
-I*|-M*|-o|-c) ;;
-I*|-M*|-O*|-o|-c) ;;
-*) cflags+="$arg ";;
esac
done
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, not sure it matters but I believe we should only need to build libbpf.a target

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for excluding -O? Why don't we want to be synced with how other parts of scx is built?

# This fixes an issue that was happening in some cases when building with gcc:
# relocation R_X86_64_32 against `.rodata' can not be used when making a PIE
# object; recompile with -fPIE
if [ "$cc" = "cc" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a reliable way to detect gcc. meson has compiler.has_argument() test, so maybe test whether -fPIE is supported there and add that to build flags?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or let's just always build with -fPIE to minimize the build complexity / variance?

Comment on lines 22 to 24
if [ "$cc" = "cc" ]; then
cflags+="-fPIE "
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reads to me it's position independent but can't be linked to create dynamic libs. No idea why it's needed.


for arg in ${args[@]:(idx+1)}; do
case $arg in
-I*|-M*|-o|-c) ;;
-I*|-M*|-O*|-o|-c) ;;
-*) cflags+="$arg ";;
esac
done
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for excluding -O? Why don't we want to be synced with how other parts of scx is built?

This is to fix an error sometimes seen when compiling with gcc,
whereby the position independent sched ext rust libraries
don't play nicely with the libbpf library if it's not also built
as position independent.

This also explicitly sets the rustc relocation-mode to "pic",
which is the default (just so this doesn't accidentally change
out from under us).
@jordalgo jordalgo changed the title Update flags when building libbpf Always build libbpf as a PIE Mar 5, 2024
@jordalgo
Copy link
Contributor Author

jordalgo commented Mar 5, 2024

Limited the scope of these changes. Luckily meson has a "pie" option for compiling executables so we'll automatically get the right flag for the selected compiler (tested with both clang and gcc).

@jordalgo
Copy link
Contributor Author

jordalgo commented Mar 5, 2024

Is there a reason for excluding -O? Why don't we want to be synced with how other parts of scx is built?

@Decave Mentioned that we should always be compiling libbpf with -O2 -- I guess the question is - why don't we do this by default for our other exes/libs?

@htejun
Copy link
Contributor

htejun commented Mar 5, 2024

Is there a reason for excluding -O? Why don't we want to be synced with how other parts of scx is built?

@Decave Mentioned that we should always be compiling libbpf with -O2 -- I guess the question is - why don't we do this by default for our other exes/libs?

Because optimized builds are slow, debugging can be painful and should by controlled by setting buildtype appropriately. I don't know whether we can make meson default to release builds, which would be fine, but it'd be annoying if the buildtype is set to debug and everything else is built accordingly but just libbpf isn't.

@Byte-Lab
Copy link
Contributor

Byte-Lab commented Mar 5, 2024

Is there a reason for excluding -O? Why don't we want to be synced with how other parts of scx is built?

@Decave Mentioned that we should always be compiling libbpf with -O2 -- I guess the question is - why don't we do this by default for our other exes/libs?

Because optimized builds are slow, debugging can be painful and should by controlled by setting buildtype appropriately. I don't know whether we can make meson default to release builds, which would be fine, but it'd be annoying if the buildtype is set to debug and everything else is built accordingly but just libbpf isn't.

Well, the counter-argument to that is that libbpf is just a dependency the same as any other whose build type we don't control. In my mind, having libbpf as a submodule was more so meant to be a convenience to avoid versioning issues than a way to control how we compile libbpf. Given how closely it interfaces with the scheduler though, I'm fine with just using the meson build flags for it.

@jordalgo jordalgo merged commit 3eb7001 into sched-ext:main Mar 5, 2024
1 check passed
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