Green jobs contain "TEST-UNEXPECTED-FAIL | check_stdcxx | We do not want these libstdc++ symbols to be used" spam

RESOLVED FIXED in Firefox 34

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: cbook, Assigned: glandium)

Tracking

(Blocks: 1 bug, {intermittent-failure})

unspecified
mozilla35
x86
Linux
intermittent-failure
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox33 unaffected, firefox34 fixed, firefox35 fixed, firefox-esr31 unaffected)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Linux x86-64 mozilla-inbound asan build on 2014-08-27 01:09:38 PDT for push 66e350a413a6

slave: b-linux64-hp-0034

https://tbpl.mozilla.org/php/getParsedLog.php?id=46840896&tree=Mozilla-Inbound



TEST-UNEXPECTED-FAIL | check_stdcxx | We do not want these libstdc++ symbols to be used:
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
These appear in green jobs & more than just ASAN.

Comes from:
http://hg.mozilla.org/mozilla-central/annotate/47c9418fbc28/config/config.mk#l693
697 ifeq (,$(filter $(OS_TARGET),WINNT Darwin))
698 CHECK_TEXTREL = @$(TOOLCHAIN_PREFIX)readelf -d $(1) | grep TEXTREL > /dev/null && echo 'TEST-UNEXPECTED-FAIL | check_textrel | We do not want text relocations in libraries and programs' || true
699 endif 

-> Bug 956398

Mike, the TEST-UNEXPECTED-FAILs aren't making the job fail (there are lots of green jobs that all contain these failures). Please can we switch to $(error foo) instead?
Blocks: 956398, 910614
Flags: needinfo?(mh+mozilla)
Summary: Intermittent ASAN | check_stdcxx | We do not want these libstdc++ symbols to be used: → Green jobs contain "TEST-UNEXPECTED-FAIL | check_stdcxx | We do not want these libstdc++ symbols to be used" spam
(Assignee)

Comment 6

4 years ago
Why are those not turning red or orange? IIRC that's what happens on "normal" builds.
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] (out from Sep 6 to Sep 22) from comment #6)
> Why are those not turning red or orange? IIRC that's what happens on
> "normal" builds.

The run isn't aborted, so is returning with a zero return code. This leave mozharness/buildbot to detect the TEST-UNEXPECTED-FAIL, which is not reliable (and something that we should/are going to move away from). I think the mozharness cases that catch TEST-UNEXPECTED-FAIL only apply to test jobs, not builds. 

Either way, if this is a fatal failure, please can you make it either abort the run or else return non-zero at the end of compile? :-)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 10

4 years ago
Note that "fixing" those to deliberately turn the build red would turn ASAN builds perma-red. Someone needs to make those messages go away first, and afaict, the necessary symbols *are* in stdc++compat, so it might be a linkage order problem. Who's the owner of asan builds these days?
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Agreed the existing failures need fixing before this is fatal, but in the meantime can we switch this to a warning that doesn't trigger a false positive with the log parser? (not being fatal but matching the fatal error regex is the worst of both worlds from a confusion to devs/sheriffs pov,  since people miss real errors in runs and mis-star)
(Assignee)

Comment 14

4 years ago
The thing is those are *not* false positives in other builds.
Created attachment 8481116 [details] [diff] [review]
Remove TEST-UNEXPECTED-FAIL from non-fatal error messages

Until check_stdcxx & check_textrel failures make the run fail,
use a failure string that won't cause log parser false positives.

(Appears in error summary means the failure was fatal. Whilst being fatal is the desired end-state, that's not the current situation.)
Attachment #8481116 - Flags: review?(mh+mozilla)
Assignee: nobody → emorley
Status: NEW → ASSIGNED
(In reply to Mike Hommey [:glandium] (out from Sep 6 to Sep 22) from comment #14)
> The thing is those are *not* false positives in other builds.

Yeah but if a real failure did occur in the other builds, it still wouldn't turn the run orange.

Let's swap the failure message for now & you can take a look at making this fatal in another bug :-)
(Assignee)

Comment 17

4 years ago
Comment on attachment 8481116 [details] [diff] [review]
Remove TEST-UNEXPECTED-FAIL from non-fatal error messages

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

Builds *do* turn orange with the current code:
https://tbpl.mozilla.org/?tree=Try&rev=c29e3f25a3d4

Your patch would completely break that.
Attachment #8481116 - Flags: review?(mh+mozilla) → review-
Oh weird. The ASAN jobs shouldn't be that different.
Either way, I think a $(error foo) approach here would be best, once the problem is fixed for ASAN jobs - since relying on a string making the run fail (vs a non-zero return code) is something that is we want to move away from.
Assignee: emorley → nobody
Attachment #8481116 - Attachment is obsolete: true
Status: ASSIGNED → NEW
Flags: needinfo?(choller)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
The PKIX ASan failures with these "SUMMARY: AddressSanitizer: use-after-poison" should be marked as bug 1039166.
This is a very recent regression, I was able to bisect it locally:


The first bad revision is:
changeset:   201640:2e3703f9be76
user:        Jacek Caban
date:        Tue Aug 26 13:50:27 2014 +0200
summary:     Bug 1055627 - Pass MOZ_GLUE_PROGRAM_LDFLAGS after STATIC_LIBS to linker. r=glandium


Mike, can you please check how the patch broke this and recommend a fix? Thanks.
Blocks: 1055627
Flags: needinfo?(choller) → needinfo?(mh+mozilla)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 25

4 years ago
Try removing NO_EXPAND_LIBS = 1 in build/unix/stdc++compat/Makefile.in and see if that removes those messages.
Flags: needinfo?(mh+mozilla)
I commented that line out, but now it doesn't build anymore. I get this:


16:40.24 /usr/bin/ld.gold.real: error: /srv/repos/browser/mozilla-central/objdir-ff-asan64opt/toolkit/library/../../build/unix/stdc++compat/stdc++compat.o: multiple definition of 'std::ctype<char>::_M_widen_init() const'
16:40.24 /usr/bin/ld.gold.real: /srv/repos/browser/mozilla-central/objdir-ff-asan64opt/toolkit/library/../../build/unix/stdc++compat/stdc++compat.o: previous definition here
16:40.24 /usr/bin/ld.gold.real: error: /srv/repos/browser/mozilla-central/objdir-ff-asan64opt/toolkit/library/../../build/unix/stdc++compat/stdc++compat.o: multiple definition of 'std::__detail::_List_node_base::_M_reverse()'
16:40.24 /usr/bin/ld.gold.real: /srv/repos/browser/mozilla-central/objdir-ff-asan64opt/toolkit/library/../../build/unix/stdc++compat/stdc++compat.o: previous definition here
16:40.24 /usr/bin/ld.gold.real: error: /srv/repos/browser/mozilla-central/objdir-ff-asan64opt/toolkit/library/../../build/unix/stdc++compat/stdc++compat.o: multiple definition of 'std::__detail::_List_node_base::_M_transfer(std::__detail::_List_node_base*, std::__detail::_List_node_base*)'


plus a few more of the same kind, when linking libxul.so.
Flags: needinfo?(mh+mozilla)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 31

4 years ago
Created attachment 8485378 [details] [diff] [review]
Stop building stdc++compat as a real library

Incidentally, this makes the build fail because stdc++compat.o then appears
twice on the libxul linkage command line. This, in turn, is because
widget/xremoteclient creates both a program and an intermediate library for
libxul. The Program() template adds stdc++compat to the directory data,
which ends up being added to libxul as well.

While eventually we'll be able to have Programs and Libraries in the same
directory without such side effects, it's not the case yet, so separate
them out for now.
Attachment #8485378 - Flags: review?(mshal)
(Assignee)

Updated

4 years ago
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 33

4 years ago
Created attachment 8485384 [details] [diff] [review]
Make stdc++compat errors fatal
Attachment #8485384 - Flags: review?(mshal)
(Assignee)

Comment 34

4 years ago
Comment on attachment 8485378 [details] [diff] [review]
Stop building stdc++compat as a real library

Gah, that fails linking gtest-libxul for a similar reason.
Attachment #8485378 - Flags: review?(mshal)
(Assignee)

Comment 35

4 years ago
Created attachment 8485393 [details] [diff] [review]
Stop building stdc++compat as a real library

Incidentally, this makes the build fail because stdc++compat.o then appears
twice on the libxul linkage command line. This, in turn, is because
widget/xremoteclient creates both a program and an intermediate library for
libxul. The Program() template adds stdc++compat to the directory data,
which ends up being added to libxul as well. The same kind of issue arises
when linking the gtest-enabled libxul.

While eventually we'll be able to avoid those problems, it's not the case
yet, so work around the issue by making expand-libs skip .desc files that
appear multiple times.

https://tbpl.mozilla.org/?tree=Try&rev=f6c08d75e636
Attachment #8485393 - Flags: review?(mshal)
(Assignee)

Updated

4 years ago
Attachment #8485378 - Attachment is obsolete: true
Comment hidden (Treeherder Robot)
Comment on attachment 8485384 [details] [diff] [review]
Make stdc++compat errors fatal

It might be simpler if you use -v to invert the return code of the first grep, since you wouldn't need the 'true' at the end, so 'false' could work as expected.
Attachment #8485384 - Flags: review?(mshal) → review+
Comment on attachment 8485393 [details] [diff] [review]
Stop building stdc++compat as a real library

>         if os.path.exists(arg + conf.LIBS_DESC_SUFFIX):
>-            with open(arg + conf.LIBS_DESC_SUFFIX, 'r') as f:
>+            desc = os.path.abspath(arg + conf.LIBS_DESC_SUFFIX)
>+            if desc in self._descs:
>+                return []
>+            self._descs.add(desc)
>+            with open(desc, 'r') as f:

Any reason not to move desc above the first if? Eg:

desc = os.path.abspath(arg + conf.LIBS_DESC_SUFFIX)
if os.path.exists(desc):
    if desc in self._descs:
...
Attachment #8485393 - Flags: review?(mshal) → review+
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Reporter)

Comment 43

4 years ago
https://hg.mozilla.org/mozilla-central/rev/00ddd6873d65
https://hg.mozilla.org/mozilla-central/rev/1a7fb15552ab
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
status-firefox33: --- → unaffected
status-firefox34: --- → affected
status-firefox35: --- → fixed
status-firefox-esr31: --- → unaffected
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
You need to log in before you can comment on or make changes to this bug.