Skip to content

Fix link issue of test_libhttp2 on FreeBSD 12 with --enable-debug #6666

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.

Sign up for GitHub

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

Jump to bottom
Merged
zwoop merged 1 commit into from
Apr 21, 2020
Merged

Fix link issue of test_libhttp2 on FreeBSD 12 with --enable-debug #6666

zwoop merged 1 commit into from
Apr 21, 2020

Conversation

@masaori335
Copy link
Contributor

@masaori335 masaori335 commented Apr 16, 2020 •
edited
Loading

Fix FreeBSD 12 debug jobs on ci.

libtool: link: ccache clang++ -std=c++17 -ggdb3 -pipe -Wall -Wno-deprecated-declarations -Qunused-arguments -Wextra -Wno-ignored-qualifiers -Wno-unused-parameter -Werror -Wno-invalid-offsetof -mcx16 -Qunused-arguments -rdynamic -Wl,-R/usr/local/lib -Wl,-R/usr/local/lib -o .libs/test_libhttp2 unit_tests/test_libhttp2-test_Http2Frame.o unit_tests/test_libhttp2-main.o  -L/usr/local/lib -L/usr/lib libhttp2.a ../../proxy/hdrs/libhdrs.a ../../src/tscore/.libs/libtscore.so -L/var/jenkins/workspace/freebsd_12-9.0.x/compiler/clang/label/freebsd_12/type/debug/111/build/BUILDS/lib/yamlcpp /var/jenkins/workspace/freebsd_12-9.0.x/compiler/clang/label/freebsd_12/type/debug/111/build/BUILDS/src/tscpp/util/.libs/libtscpputil.so -lpcre -lssl -lcrypto ../../src/tscpp/util/.libs/libtscpputil.so ../../iocore/eventsystem/libinkevent.a ../../lib/records/librecords_p.a ../../mgmt/.libs/libmgmt_p.a ../../proxy/shared/libUglyLogStubs.a -lexecinfo -lpthread -Wl,-rpath -Wl,/var/jenkins/workspace/freebsd_12-9.0.x/compiler/clang/label/freebsd_12/type/debug/111/install/lib -Wl,-rpath -Wl,/usr/lib -Wl,-rpath -Wl,/usr/local/lib
ld: error: undefined symbol: ProxySession::ProxySession()
>>> referenced by Http2ClientSession.cc:68 (../../../proxy/http2/Http2ClientSession.cc:68)
>>>               Http2ClientSession.o:(Http2ClientSession::Http2ClientSession()) in archive libhttp2.a

ld: error: undefined symbol: ProxySession::ProxySession()
>>> referenced by Http2ClientSession.cc:68 (../../../proxy/http2/Http2ClientSession.cc:68)
>>>               Http2ClientSession.o:(Http2ClientSession::Http2ClientSession()) in archive libhttp2.a
...

https://ci.trafficserver.apache.org/job/freebsd_12-master/compiler=clang,label=freebsd_12,type=debug/1334/console

Both of 9.0.x and 8.1.x branch need this. The unit test is introduced by 582df40 and it was cherry picked to 9.0.x and 8.1.x.

@masaori335 masaori335 added the Build work related to build configuration or environment label Apr 16, 2020
@masaori335 masaori335 added this to the 10.0.0 milestone Apr 16, 2020
@masaori335 masaori335 requested a review from zwoop April 16, 2020 05:42
@masaori335 masaori335 self-assigned this Apr 16, 2020
@masaori335 masaori335 marked this pull request as draft April 16, 2020 06:05
@zwoop
Copy link
Contributor

zwoop commented Apr 16, 2020

[approve ci].

@masaori335
Copy link
Contributor Author

masaori335 commented Apr 17, 2020 •
edited
Loading

I found the correct order of libs that satisfy our platforms. It's a bit wired, though.

@masaori335 masaori335 marked this pull request as ready for review April 17, 2020 02:21
@maskit
Copy link
Member

maskit commented Apr 17, 2020

Does this mean librecords_p.a depends on libhttp2.a? How come?
It would be nice if you could add the reason we need two lines for libhttp2 and libhdrs. Some may think those are just duplications and remove them.

@masaori335
Copy link
Contributor Author

masaori335 commented Apr 17, 2020

Add a comment.
I have no idea why LLD fails to link and this change fixes the issue. It'd great if anybody can find the root cause.

@zwoop
Copy link
Contributor

zwoop commented Apr 17, 2020

This looks really clunky ... :) Not saying we shouldn't do this, but it doesn't feel right? :)

@masaori335
Copy link
Contributor Author

masaori335 commented Apr 20, 2020 •
edited
Loading

I agree with this is an ugly hack:(

I still don't know what is root cause, but this looks LLD specific issue with debug build. If I use GNU ld instead of LLD, the issue is gone.

Does this mean librecords_p.a depends on libhttp2.a?

No. LLD has a different approach of handling of archive files, the order doesn't mean dependencies.

Instead of memorizing only undefined symbols, we program LLD so that it memorizes all symbols. When it sees an undefined symbol that can be resolved by extracting an object file from an archive file it previously visited, it immediately extracts the file and links it. It is doable because LLD does not forget symbols it has seen in archive files.
http://lld.llvm.org/NewLLD.html#design

@maskit
Copy link
Member

maskit commented Apr 20, 2020

I looked into this too. Here's a couple of findings:

  • Optimization does some magic. If I add -O1 to AM_CPPFLAGS on http2/Makefile.am then link succeeds without the LDADD change, but not -O0.

  • I put the dup lines between libinkevent.a and librecords_p.a (instead of after librecords_p.a) and it worked too.

Because Http2ClientSession does depend on ProxySession, adding libproxy.a or a stub (mock) module that satisfies the requirement for libproxy.a may be an appropriate approach.

@masaori335
Copy link
Contributor Author

masaori335 commented Apr 21, 2020

Thanks for looking.

Optimization does some magic. If I add -O1 to AM_CPPFLAGS on http2/Makefile.am then link succeeds without the LDADD change, but not -O0.

This explains why we are facing the issue with debug build. Some symbols which are removed by optimization would be a trigger.

I put the dup lines between libinkevent.a and librecords_p.a (instead of after librecords_p.a) and it worked too.

The order around libinkevent.a looks matter for LLD. This is another peace.
I tried another order, moving libinkevent.a between libhttp2.a and libhdrs.a. It also works.

@masaori335
Copy link
Contributor Author

masaori335 commented Apr 21, 2020

I pushed new commit which satisfies both of GNU ld & LLD without duplicated lines.

randall
randall previously approved these changes Apr 21, 2020
View reviewed changes
@maskit
Copy link
Member

maskit commented Apr 21, 2020

I tried another order, moving libinkevent.a between libhttp2.a and libhdrs.a. It also works.

This is interesting. You may already have tried this, but adding only a dup libhdr.a anywhere after libinkevent.a works too (checked only with LLD). I thought libhttp2.a is the trigger but it might be libhdr.a.

The latest change, which moves libinkevent.a between libhttp2.a and libhdr.a, would be the best if I had to choose one from the options we currently have. However, we still depends on the optimization magic, which is uncontrollable. Can you put add a comment that says there is this mystery? A link for this issue would be helpful.

@masaori335
Copy link
Contributor Author

masaori335 commented Apr 21, 2020

[approve ci centos]

@zwoop zwoop merged commit 3a6f49b into apache:master Apr 21, 2020
@zwoop zwoop modified the milestones: 10.0.0, 8.1.0 Apr 21, 2020
@zwoop
Copy link
Contributor

zwoop commented Apr 21, 2020

Cherry-picked to 8.1.x
Cherry-picked to v9.0.x branch.

@brbzull0 brbzull0 mentioned this pull request Dec 3, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maskit maskit maskit approved these changes

@bryancall bryancall bryancall approved these changes

@randall randall randall left review comments

@zwoop zwoop Awaiting requested review from zwoop

Assignees

@masaori335 masaori335

Labels

Build work related to build configuration or environment

Projects

None yet

Milestone

8.1.0

Development

Successfully merging this pull request may close these issues.

5 participants

@masaori335 @zwoop @maskit @bryancall @randall