Closed Bug 1264534 Opened 7 years ago Closed 7 years ago

Unable to link MFBT tests (missing -lm option to linker)

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox50 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: mt, Unassigned)

Details

Attachments

(4 files, 1 obsolete file)

unlinkable.txt
23.92 KB, text/plain
Details
Unified_cpp_mfbt0.i
1.30 MB, text/plain
Details
bug1264534.patch
653 bytes, patch
glandium
: feedback-
Details | Diff | Splinter Review
Bug 1264534 - Create system wrappers for all declared STL headers.
58 bytes, text/x-review-board-request
froydnj
: review+
Details 
Files in the double-conversion directory use `ceil` and include <math.h>, but I'm not seeing -lm on the command line.

 0:06.63 Executing: clang++ -Qunused-arguments -Qunused-arguments -Wall -Wc++11-compat -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wc++11-compat-pedantic -Wc++14-compat -Wc++14-compat-pedantic -Wc++1z-compat -Wclass-varargs -Wimplicit-fallthrough -Wloop-analysis -Wthread-safety -Wno-invalid-offsetof -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-unknown-warning-option -Wno-return-type-c-linkage -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++11 -pthread -pipe -g -fno-omit-frame-pointer -o TestArrayUtils /home/martin/code/obj/bug1264470/default/mfbt/tests/tmpeWfr_g.list -lpthread -Wl,-z,noexecstack -Wl,-z,text -Wl,--build-id -B ../../build/unix/gold -Wl,-rpath-link,/home/martin/code/obj/bug1264470/default/dist/bin -Wl,-rpath-link,/usr/local/lib -ldl
 0:06.63 /home/martin/code/obj/bug1264470/default/mfbt/tests/tmpeWfr_g.list:
 0:06.63     INPUT("TestArrayUtils.o")
 0:06.63     INPUT("../Compression.o")
 0:06.63     INPUT("../Decimal.o")
 0:06.63     INPUT("../Unified_cpp_mfbt0.o")
 0:06.63
 0:06.63 /home/martin/code/gecko-dev/mfbt/double-conversion/bignum-dtoa.cc:410: error: undefined reference to 'ceil'
 0:06.63 /home/martin/code/gecko-dev/mfbt/double-conversion/cached-powers.cc:148: error: undefined reference to 'ceil'
OK, tip of the iceberg.  Firefox fails to link too.
Component: MFBT → Build Config
The compiler should be adding -lm on its own.

Try reexecuting that clang++ command with -v, it will tell you how it invokes ld, there should be a -lm on that command.
Yep, the linker is invoked with -lm, right after -lstdc++:

 "../../build/unix/gold/ld" --hash-style=gnu --no-add-needed --build-id --eh-frame-hdr -m elf_x86_64 -dynamic-linker /lib64/ld-linux-x86-64.so.2 -o TestArrayUtils /usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0/../../../../lib64/crt1.o /usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0/../../../../lib64/crti.o /usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0/crtbegin.o -L/usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0 -L/usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0/../../../../lib64 -L/usr/bin/../lib64 -L/lib/../lib64 -L/usr/lib/../lib64 -L/usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0/../../.. -L/usr/bin/../lib -L/lib -L/usr/lib /home/martin/code/obj/bug1264470/default/mfbt/tests/tmpdSYQSo.list -lpthread -z noexecstack -z text --build-id -rpath-link /home/martin/code/obj/bug1264470/default/dist/bin -rpath-link /usr/local/lib -ldl -lstdc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc /usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0/crtend.o /usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0/../../../../lib64/crtn.o

Building a simple test program works fine:

#include <math.h>
int main() { return ceil(0.4f); }

So it's got to be in there somewhere.
Try with -Wl,--verbose, it will make ld send more output
Attached file unlinkable.txt — 
Looks like it's finding libm, and libm seems to both exist and have a symbol for ceil.
OK, because I saw libm in there once, I didn't notice:

../../build/unix/gold/ld: Attempt to open /usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0/libm.so failed
../../build/unix/gold/ld: Attempt to open /usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0/libm.a failed

Which really doesn't exist.
Of course, it is eventually found, just like libdl
So at this point I'm ready to say it's likely a visibility problem.

What does `objdump -t /home/martin/code/obj/bug1264470/default/mfbt/Unified_cpp_mfbt0.o | grep ceil` say?
$ objdump -t /home/martin/code/obj/bug1264470/default/mfbt/Unified_cpp_mfbt0.o | grep ceil
0000000000000000         *UND*	0000000000000000 .hidden ceil
Okay, so, now, we need to figure out why it's has that .hidden. Can you `make -C /home/martin/code/obj/bug1264470/default/mfbt/ Unified_cpp_mfbt0.i` and attach the resulting file? (or, if you know what to look for, you can also look for it yourself)
Attached file Unified_cpp_mfbt0.i — 
I'm seeing ceil defined as `inline constexpr` (/usr/include/c++/6.0.0/cmath) which points to __builtin_ceil*().  But there is also an `extern` declaration (/usr/include/bits/mathcalls.h).
So, the one that matters is the one in mathcalls.h, which is included from math.h, which is included by cmath.

Neither cmath or math.h is included through a system wrapper... which is weird. math.h *is* in config/system-headers, so you should have a math.h file in /home/martin/code/obj/bug1264470/default/dist/system_wrappers... If you do, the question becomes why is your compiler not including it? If you don't, the question becomes why is there no math.h file there?
config/system_wrappers/math.h exists:

#pragma GCC system_header
#pragma GCC visibility push(default)
#include_next <math.h>
#pragma GCC visibility pop
The first inclusion of math.h goes straight to /usr/include/math.h, which is via cmath:

# 1 "/home/martin/code/obj/bug1264470/default/dist/include/mozilla/MathAlgorithms.h" 1
# 1 "/usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0/../../../../include/c++/6.0.0/cmath" 1 3
# 40 "/usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0/../../../../include/c++/6.0.0/cmath" 3
# 1 "/usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0/../../../../include/c++/6.0.0/bits/cpp_type_trait
s.h" 1 3
# 36 "/usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0/../../../../include/c++/6.0.0/bits/cpp_type_trai
ts.h" 3
# 275 "/usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0/../../../../include/c++/6.0.0/bits/cpp_type_tra
its.h" 3
# 43 "/usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0/../../../../include/c++/6.0.0/cmath" 2 3
# 1 "/usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0/../../../../include/c++/6.0.0/ext/type_traits.h"
1 3
# 33 "/usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0/../../../../include/c++/6.0.0/ext/type_traits.h"
 3
# 44 "/usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0/../../../../include/c++/6.0.0/cmath" 2 3
# 1 "/usr/include/math.h" 1 3 4

The problem seems to be glibc again... from the above in <cmath>:

#define _GLIBCXX_INCLUDE_NEXT_C_HEADERS
#include_next <math.h>
#undef _GLIBCXX_INCLUDE_NEXT_C_HEADERS

Name        : glibc-devel
Version     : 2.23.90
Release     : 8.fc25
So I'm getting a successful build by patching /usr/include/c++/6.0.0/cmath to using #include rather than #include_next.

It appears as though the intent with using #include_next here was to skip inclusion of /usr/include/c++/6.0.0/math.h but it has the impact of also skipping our system_wrappers/math.h, which includes the critical `#pragma GCC visibility push(default)` directive.

Given that the #include(_next) is wrapped in defines for _GLIBCXX_INCLUDE_NEXT_C_HEADERS, which then route to an #include_next in /usr/include/c++/6.0.0/math.h, it seems like the #include_next in <cmath> was an unnecessary optimization.
Attached patch bug1264534.patch — — Splinter Review 
:glandium, does this make sense to you?

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70722#c2

It didn't work for me.
Attachment #8742739 - Flags: feedback?(mh+mozilla)
Comment on attachment 8742739 [details] [diff] [review]
bug1264534.patch

Review of attachment 8742739 [details] [diff] [review]:
-----------------------------------------------------------------

I think I have a better fix.
Attachment #8742739 - Flags: feedback?(mh+mozilla) → feedback-
So, it turns out there should be nothing to fix... so I suspect something fishy. Are you trying to outsmart the build system and run targeted mach/make commands? Are you using ccache? What exact commands lead to the failure if you start from a clobber?
./mach clobber;./mach build

ccache is not installed

.mozconfig:

mk_add_options MOZ_OBJDIR=$MOZ_OBJDIR
mk_add_options AUTOCLOBBER=1

ac_add_options --disable-optimize
ac_add_options --enable-debug

ac_add_options --disable-gamepad
ac_add_options --disable-universalchardet
ac_add_options --disable-necko-wifi
ac_add_options --disable-webapp-runtime
ac_add_options --disable-webspeech
ac_add_options --without-intl-api
ac_add_options --disable-updater
So, how are you ending up not including dist/stl_wrappers/cmath? That's what MathAlgorithms.h should end up including when building Unified_cpp_mfbt0.cpp, and that's what happens on my machine.
clang -cc1 version 3.8.0 based upon LLVM 3.8.0 default target x86_64-unknown-linux-gnu
ignoring nonexistent directory "/include"
#include "..." search starts here:
#include <...> search starts here:
 /home/martin/code/obj/bug1264470/default/dist/system_wrappers
 /home/martin/code/gecko/mfbt
 /home/martin/code/obj/bug1264470/default/mfbt
 /home/martin/code/obj/bug1264470/default/dist/include
 /home/martin/code/obj/bug1264470/default/dist/include/nspr
 /home/martin/code/obj/bug1264470/default/dist/include/nss
 /usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0/../../../../include/c++/6.0.0
 /usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0/../../../../include/c++/6.0.0/x86_64-redhat-linux
 /usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0/../../../../include/c++/6.0.0/backward
 /usr/local/include
 /usr/bin/../lib64/clang/3.8.0/include
 /usr/include
End of search list.
Oops, missed this:

 "/usr/bin/clang-3.8" -cc1 -triple x86_64-unknown-linux-gnu -E -disable-free -disable-llvm-verifier -main-file-name Unified_cpp_mfbt0.cpp -mrelocation-model pic -pic-level 2 -mthread-model posix -mdisable-fp-elim -relaxed-aliasing -masm-verbose -mconstructor-aliases -munwind-tables -fuse-init-array -target-cpu x86-64 -v -dwarf-column-info -debug-info-kind=limited -dwarf-version=4 -debugger-tuning=gdb -resource-dir /usr/bin/../lib64/clang/3.8.0 -C -dependency-file .deps/Unified_cpp_mfbt0.i.pp -MT Unified_cpp_mfbt0.i -sys-header-deps -MP -include /home/martin/code/gecko/config/gcc_hidden.h -include /home/martin/code/obj/bug1264470/default/mozilla-config.h -I /home/martin/code/obj/bug1264470/default/dist/system_wrappers -D DEBUG=1 -D TRACING=1 -D IMPL_MFBT -I /home/martin/code/gecko/mfbt -I /home/martin/code/obj/bug1264470/default/mfbt -I /home/martin/code/obj/bug1264470/default/dist/include -I /home/martin/code/obj/bug1264470/default/dist/include/nspr -I /home/martin/code/obj/bug1264470/default/dist/include/nss -D MOZILLA_CLIENT -internal-isystem /usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0/../../../../include/c++/6.0.0 -internal-isystem /usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0/../../../../include/c++/6.0.0/x86_64-redhat-linux -internal-isystem /usr/bin/../lib/gcc/x86_64-redhat-linux/6.0.0/../../../../include/c++/6.0.0/backward -internal-isystem /usr/local/include -internal-isystem /usr/bin/../lib64/clang/3.8.0/include -internal-externc-isystem /include -internal-externc-isystem /usr/include -Wall -Wc++11-compat -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wc++11-compat-pedantic -Wc++14-compat -Wc++14-compat-pedantic -Wc++1z-compat -Wclass-varargs -Wimplicit-fallthrough -Wloop-analysis -Wthread-safety -Wno-invalid-offsetof -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-unknown-warning-option -Wno-return-type-c-linkage -std=gnu++11 -fdeprecated-macro -fdebug-compilation-dir /home/martin/code/obj/bug1264470/default/mfbt -ferror-limit 19 -fmessage-length 100 -pthread -fno-rtti -fobjc-runtime=gcc -fdiagnostics-show-option -o Unified_cpp_mfbt0.i -x c++ /home/martin/code/obj/bug1264470/default/mfbt/Unified_cpp_mfbt0.cpp
I'm not seeing a -I for stl_wrappers.

~/code/obj/bug1264470/default
$ find . -type f -exec grep STL_FLAGS {} \+
Binary file ./config.statusc matches
./config/autoconf.mk:STL_FLAGS = -I/home/martin/code/obj/bug1264470/default/dist/stl_wrappers
./config.status:    "STL_FLAGS": "-I/home/martin/code/obj/bug1264470/default/dist/stl_wrappers",

Nothing in backend.mk or other makefiles.
./mfbt/mozbuild:119:DISABLE_STL_WRAPPING = True

So we get wrapping of <math.h> but not <cmath> according to this:

./config.mk:332:COMPILE_CXXFLAGS = $(if $(DISABLE_STL_WRAPPING),,$(STL_FLAGS)) $(VISIBILITY_FLAGS) $(DEFINES) $(INCLUDES) $(OS_INCLUDES) $(DSO_CFLAGS) $(DSO_PIC_CFLAGS) $(RTL_FLAGS) $(OS_COMPILE_CXXFLAGS) $(_DEPEND_CFLAGS) $(CXXFLAGS) $(MOZBUILD_CXXFLAGS)

Is that it?  I'm going to move VISIBILITY_FLAGS under the DISABLE_STL_WRAPPING conditional and see what happens.
Oh sorry, I got cmath wrapping locally because of the wrong reason.

I'll need to do some archeology, but I think we could remove DISABLE_STL_WRAPPING in mfbt/moz.build and probably a lot of other files.
When wrapping system headers to apply visibility tweaks, both the STL
and standard system headers need to be wrapped at the same time.

For instance, libstdc++ 6 <cmath> calls include_next for <math.h>.  This
results in the wrapper for <math.h> and its visibility changes are
consequently not applied.

Review commit: https://reviewboard.mozilla.org/r/47915/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47915/
Attachment #8743603 - Flags: review?(mh+mozilla)
Comment on attachment 8743603 [details]
MozReview Request: Bug 1264534 - Apply all visibility wrappers or none, r?glandium

https://reviewboard.mozilla.org/r/47915/#review44645

I don't think this is the right thing to do. Please try removing DISABLE_STL_WRAPPERS.
Attachment #8743603 - Flags: review?(mh+mozilla)
Ack.  It doesn't work for /js/src anyway, since it doesn't define STL_FLAGS.
A simple excision of DISABLE_STL_WRAPPING doesn't work.  It appears that the flag is most often used with the intent to disable jemalloc.  Also ipc/chromium/src/base fails with:

12:33.03 #    error "STL code can only be used with infallible ::operator new()"
I didn't say to remove it everywhere.
OK, I successfully removed DISABLE_STL_WRAPPING from mfbt and its tests subdir.  Doing so required that I link mozglue for the tests, and to remove a re-definition of InfallibleAllocPolicy.  That seemed to work OK for mfbt.

There are widespread hidden symbols left over though:

$ find . -name '*.o' -type f -print | while read a; do objdump -t "$a" | grep '.hidden ceil' > /dev/null && echo $a; done
./js/src/jsutil.o
[... more ./js stuff]
./media/webrtc/trunk/webrtc/common_audio/common_audio_common_audio/Unified_cpp_webrtc_common_audio0.o
./media/webrtc/trunk/webrtc/modules/modules_remote_bitrate_estimator/Unified_cpp_webrtc_modules0.o
./media/webrtc/trunk/webrtc/modules/modules_rtp_rtcp/Unified_cpp_webrtc_modules1.o

$ find . -name '*.o' -type f -print | while read a; do objdump -t "$a" | grep '.hidden floor' > /dev/null && echo $a; done
./js/src/Unified_cpp_js_src12.o
[... more ./js stuff]
./media/webrtc/trunk/webrtc/modules/modules_audio_processing/Unified_cpp_webrtc_modules0.o
./media/webrtc/trunk/webrtc/modules/modules_audio_processing/histogram.o
./media/libvorbis/vorbis_floor1.o
./media/libvorbis/Unified_c_media_libvorbis0.o

$ find . -name '*.o' -type f -print | while read a; do objdump -t "$a" | grep '.hidden sin$' > /dev/null && echo $a; done
./js/src/jsmath.o
./js/src/Unified_cpp_js_src1.o
./media/webrtc/trunk/webrtc/common_audio/common_audio_common_audio/Unified_cpp_webrtc_common_audio0.o
./media/webrtc/trunk/webrtc/modules/modules_audio_processing/covariance_matrix_generator.o

JS doesn't use STL_FLAGS at all, so that's a tricky one to fix.  I have no idea on what to do with the media stuff.
I'm hitting this same issue locally in a Linux clang build. A clobber didn't fix it.
On Ubuntu 16.04, I was able to patch /usr/include/c++/6.1.1/cmath just as described in comment 15, but messing with the system headers isn't a good solution I guess.

We need this to be fixed because we depend on being able to make builds on Ubuntu with Clang experimental features (including AFL).
Until now, we had some STL headers listed in the system-headers list
such that they would get a system wrapper, but not all of them.

On the other hand, the STL wrappers do the same job as the system
wrappers (applying default visibility), on top of their own, so the
presence of the STL headers in system-headers wasn't making much of a
difference.

Except we have places in the tree where we can't build with STL wrappers
for a number of reasons. And in that case, we *do* need system wrappers
for the STL headers, but we didn't have system wrappers for all of them.

So instead of relying on the STL headers being listed both in
system-headers and stl-headers, concatenate both lists to create the
system wrappers.

Review commit: https://reviewboard.mozilla.org/r/59270/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59270/
Attachment #8762792 - Flags: review?(nfroyd)
Attachment #8743603 - Attachment is obsolete: true
Comment on attachment 8762792 [details]
Bug 1264534 - Create system wrappers for all declared STL headers.

https://reviewboard.mozilla.org/r/59270/#review56294

Sure.
Attachment #8762792 - Flags: review?(nfroyd) → review+
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1cac03485d9
Create system wrappers for all declared STL headers. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/e1cac03485d9
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.