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

go native: Add support for linking static libs #9063

Closed
wants to merge 1 commit into from
Closed

go native: Add support for linking static libs #9063

wants to merge 1 commit into from

Conversation

pjbgf
Copy link
Contributor

@pjbgf pjbgf commented Nov 24, 2022

Some specific projects (FluxCD) may require the linking of static libraries. The new environment variable
ADDITIONAL_LIBS enables multiple space separated libs to be linked.

This is to remove the current hack within the Flux project that enables the linking of libgit2.a during fuzz compilation.

@AdamKorcz
Copy link
Collaborator

AdamKorcz commented Nov 24, 2022

This would be nice to add.

@DavidKorczynski
Copy link
Collaborator

DavidKorczynski commented Nov 24, 2022

Would it make sense to do this by simple extending CFLAGS, CXXFLAGS or LIB_FUZZING_ENGINE instead of introducing a new environment variable?

e.g.

CFLAGS="${CFLAGS} -libgit2.a" compile_native_go_fuzzer

Copy link
Collaborator

@AdamKorcz AdamKorcz left a comment

Choose a reason for hiding this comment

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

Just tried running this on the Helm project and am getting /usr/local/bin/compile_native_go_fuzzer: line 33: ADDITIONAL_LIBS: unbound variable

Does what David suggest above solve your problem?

@pjbgf
Copy link
Contributor Author

pjbgf commented Nov 25, 2022

@DavidKorczynski I tried a few approaches:
a) CFLAGS and CXXFLAGS, using -L and -l flags, as well as full path to the statically built lib.
b) LIB_FUZZING_ENGINE using the full path to the statically built lib.

The only way I got this to work was to pass on the statically built lib as the last arg.
I am far from being an expert on C/CXX Flags, so happy to try other suggestions if you may have. So keep them coming. :)

For the fuzzing we don't really activate the execution paths that touch libgit2, so maybe we could even ignore undefined symbols. But I could not get that to work when building the fuzzers.

@pjbgf
Copy link
Contributor Author

pjbgf commented Nov 25, 2022

@AdamKorcz I now ensure that the new var has a default value. With the latest, this seem to work against fluxcd and helm.

For flux I tested this in combination with other changes I am submitting to the project.

Some specific projects (FluxCD) may require the linking
of static libraries. The new environment variable
ADDITIONAL_LIBS enables multiple space separated libs to
be linked.

Signed-off-by: Paulo Gomes <pjbgf@linux.com>
@DavidKorczynski
Copy link
Collaborator

I am far from being an expert on C/CXX Flags, so happy to try other suggestions if you may have. So keep them coming. :)

Sounds good -- I'll give it a try shortly

@pjbgf
Copy link
Contributor Author

pjbgf commented Nov 25, 2022

@DavidKorczynski thank you. For a quicker test, you may want to leave only source-controller in here.

The location for the lib we depend on is:
/root/go/src/github.com/fluxcd/source-controller/build/libgit2/v0.4.0/lib/libgit2.a

@DavidKorczynski
Copy link
Collaborator

DavidKorczynski commented Nov 25, 2022

@DavidKorczynski thank you. For a quicker test, you may want to leave only source-controller in here.

The location for the lib we depend on is: /root/go/src/github.com/fluxcd/source-controller/build/libgit2/v0.4.0/lib/libgit2.a

Thanks @pjbgf -- I think I have a solution. If you add this to your build.sh you should have a succesful build:

GOPATH="${GOPATH:-/root/go}"
ORG_ROOT="${ORG_ROOT:-${GOPATH}/src/github.com/fluxcd}"
PREBUILD_SCRIPT_PATH="${PREBUILD_SCRIPT_PATH:-tests/fuzz/oss_fuzz_prebuild.sh}"
POSTBUILD_SCRIPT_PATH="${POSTBUILD_SCRIPT_PATH:-tests/fuzz/oss_fuzz_postbuild.sh}"
FLUX_CI="${FLUX_CI:-false}"

LIBGIT2_TAG="${LIBGIT2_TAG:-v0.4.0}"
GO_SRC="${GOPATH}/src"
PROJECT_PATH="github.com/fluxcd/source-controller"
pushd "${GO_SRC}/${PROJECT_PATH}"
export TARGET_DIR="$(/bin/pwd)/build/libgit2/${LIBGIT2_TAG}"
# For most cases, libgit2 will already be present.
# The exception being at the oss-fuzz integration.
if [ ! -d "${TARGET_DIR}" ]; then
    curl -o output.tar.gz -LO "https://github.com/fluxcd/golang-with-libgit2/releases/download/${LIBGIT2_TAG}/linux-x86_64-libgit2-only.tar.gz"

    DIR=linux-libgit2-only
    NEW_DIR="$(/bin/pwd)/build/libgit2/${LIBGIT2_TAG}"
    INSTALLED_DIR="/home/runner/work/golang-with-libgit2/golang-with-libgit2/build/${DIR}"
    mkdir -p ./build/libgit2
    tar -xf output.tar.gz
    rm output.tar.gz
    mv "${DIR}" "${LIBGIT2_TAG}"
    mv "${LIBGIT2_TAG}/" "./build/libgit2"
    # Update the prefix paths included in the .pc files.
    # This will make it easier to update to the location in which they will be used.
    find "${NEW_DIR}" -type f -name "*.pc" | xargs -I {} sed -i "s;${INSTALLED_DIR};${NEW_DIR};g" {}
fi
export LIB_FUZZING_ENGINE="${LIB_FUZZING_ENGINE} -Wl,--start-group ${TARGET_DIR}/lib/libgit2.a"

Add it right after

GOPATH="${GOPATH:-/root/go}"
ORG_ROOT="${ORG_ROOT:-${GOPATH}/src/github.com/fluxcd}"
PREBUILD_SCRIPT_PATH="${PREBUILD_SCRIPT_PATH:-tests/fuzz/oss_fuzz_prebuild.sh}"
POSTBUILD_SCRIPT_PATH="${POSTBUILD_SCRIPT_PATH:-tests/fuzz/oss_fuzz_postbuild.sh}"
FLUX_CI="${FLUX_CI:-false}"

The problem is indeed the ordering of statically compiled libraries matter. Ideally, $LIB_FUZZING_ENGINE should be at the end of the $CXX linking, however, we hack around this by introducing --start-group. You'll see some outputs /usr/bin/ld: missing --end-group; added as last command line option which is alright (i.e. it works although it's not the prettiest to have in the output)

@DavidKorczynski
Copy link
Collaborator

@pjbgf my testing just finished and the build passed using the above

@DavidKorczynski
Copy link
Collaborator

@pjbgf just to clarify on the above logic, the key thing I did was add the libgit2.a logic from the source controller and then the following line to include it in the native go fuzzer compilation:
export LIB_FUZZING_ENGINE="${LIB_FUZZING_ENGINE} -Wl,--start-group ${TARGET_DIR}/lib/libgit2.a"

@DavidKorczynski
Copy link
Collaborator

DavidKorczynski commented Nov 25, 2022

@pjbgf -- just to further clarify, my modifications are based on your set up in PR #9064 -- I worked from that one (apologies, I actually thought I was writing in that one when writing these messages).

@pjbgf
Copy link
Contributor Author

pjbgf commented Nov 25, 2022

@DavidKorczynski that's brilliant, thank you very much. I will give this a try right away.

@pjbgf
Copy link
Contributor Author

pjbgf commented Nov 25, 2022

@DavidKorczynski it all worked, thank you very much for your help. I am making the required changes on the repositories, once they are merged we should be able to retest the other PR.

Therefore I will close this as no longer needed.

@pjbgf pjbgf closed this Nov 25, 2022
@pjbgf pjbgf deleted the additional-libs branch November 25, 2022 17:11
@DavidKorczynski
Copy link
Collaborator

That's great @pjbgf -- thanks for the patience and hints too!

That said, it may be useful to have $LIB_FUZZING_ENGINE after $fuzzer.a in the compile_native_go_fuzzer as it adds a level of flexibility when having to deal with problems of ordering. I guess we don't have to do anything atm, but if it becomes an issue then ping and we can fix it

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