Closed Bug 1399412 Opened 7 years ago Closed 7 years ago

Clang hits assertion after LZ4 1.8.0 update

Categories

(Core :: MFBT, defect)

Unspecified
FreeBSD
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: jbeich, Assigned: dimitry)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

After mozilla-central changeset 9a4077eda5d8 my build crashes:

/usr/bin/c++ -std=gnu++11 -o Compression.o -c -Iobj-x86_64-unknown-freebsd12.0/dist/system_wrappers -include config/gcc_hidden.h -DNDEBUG=1 -DTRIMMED=1 -DIMPL_MFBT -Imfbt -Iobj-x86_64-unknown-freebsd12.0/mfbt  -Iobj-x86_64-unknown-freebsd12.0/dist/include  -Iobj-x86_64-unknown-freebsd12.0/dist/include/nspr -Iobj-x86_64-unknown-freebsd12.0/dist/include/nss       -fPIC  -DMOZILLA_CLIENT -include obj-x86_64-unknown-freebsd12.0/mozilla-config.h -MD -MP -MF .deps/Compression.o.pp -Qunused-arguments    -I/usr/local/include -Qunused-arguments -Wall -Wc++11-compat -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wloop-analysis -Wc++11-compat-pedantic -Wc++14-compat -Wc++14-compat-pedantic -Wc++1z-compat -Wcomma -Wimplicit-fallthrough -Wstring-conversion -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wformat -Wno-gnu-zero-variadic-macro-arguments -Wformat-security -Wno-unknown-warning-option -Wno-return-type-c-linkage -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -pipe  -O -fno-omit-frame-pointer   -Wno-error=shadow  -Wno-unused-function mfbt/Compression.cpp
Assertion failed: (!Old || Old->getCachedLinkage() == D->getCachedLinkage()), function getLVForDecl, file /usr/src/contrib/llvm/tools/clang/lib/AST/Decl.cpp, line 1434.
c++: error: unable to execute command: Abort trap
c++: error: clang frontend command failed due to signal (use -v to see invocation)
FreeBSD clang version 5.0.0 (tags/RELEASE_500/final 312559) (based on LLVM 5.0.0svn)
Target: x86_64-unknown-freebsd12.0
Thread model: posix
InstalledDir: /usr/bin
Landry, Ryan, do you see similar crashes with Clang on OpenBSD or Linux?
Flags: needinfo?(ryanvm)
Flags: needinfo?(landry)
We haven't been seeing any crashes like that in our automation as far as I'm aware. We're still using version 3.9.0, though. Aryx, have you seen any clang crashes since the lz4 update hit inbound yesterday?
Flags: needinfo?(ryanvm) → needinfo?(aryx.bugmail)
We use a prerelease clang 5.0 on Windows and haven't seen any crashes...could be worth bisecting to see if recent 5.0 work introduced a regression.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #2)
> Aryx, have you seen any clang crashes since the lz4 update hit inbound yesterday?
No.
Flags: needinfo?(aryx.bugmail)
As noted in https://bugs.freebsd.org/222280, the program producing the assertion can be reduced to just this:

// clang -cc1 -triple x86_64 -S bug222280.cpp
namespace {
  extern "C" union LZ4_stream_u *LZ4_createStream();
  LZ4_stream_u *LZ4_createStream();
}

It's apparently a long-standing problem upstream, as there are multiple bugs about it, but no conclusive fix:

https://bugs.llvm.org/show_bug.cgi?id=18964
https://bugs.llvm.org/show_bug.cgi?id=19995
https://bugs.llvm.org/show_bug.cgi?id=21854
https://bugs.llvm.org/show_bug.cgi?id=23090
https://bugs.llvm.org/show_bug.cgi?id=33503

As a workaround, the inclusion of lz4.c can be wrapped in an extern "C" block.
Comment on attachment 8907685 [details] [diff] [review]
Work around clang assertion by wrapping lz4.c in an extern "C" block

https://treeherder.mozilla.org/#/jobs?repo=try&revision=deca51400f05

Thanks, the patch fixes my build.
Attachment #8907685 - Flags: review?(nfroyd)
Attachment #8907685 - Flags: feedback+
Comment on attachment 8907685 [details] [diff] [review]
Work around clang assertion by wrapping lz4.c in an extern "C" block

Oops, previous Try push was for FF55.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68f7d99854e7
Assignee: nobody → dimitry
Blocks: 1398021
Comment on attachment 8907685 [details] [diff] [review]
Work around clang assertion by wrapping lz4.c in an extern "C" block

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

|extern "C"| inside an anonymous namespace seems...really weird, but I guess there's no other way to do what we want here.  And if it works...
Attachment #8907685 - Flags: review?(nfroyd) → review+
BTW, please make sure this gets upstreamed so we don't end up clobbering it with some later update?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
> BTW, please make sure this gets upstreamed so we don't end up clobbering it
> with some later update?

There's nothing to upstream here; the patch is solely against mozilla-central-resident code.  I think the "upstream" in comment 5 was referring to LLVM upstream, insofar as there are many bugs open that cite the assertion in comment 0.
(In reply to Nathan Froyd [:froydnj] from comment #3)
> We use a prerelease clang 5.0 on Windows

Note that's not true. We're using a prerelease of clang *6*.
(In reply to Jan Beich from comment #1)
> Landry, Ryan, do you see similar crashes with Clang on OpenBSD or Linux?

I don't really know, build is broken on OpenBSD since #1391126 - but i'm using clang 5.0, and maybe the lz4 code is built before hitting this failure.
Flags: needinfo?(landry)
For landing, please, use attachment author/description or adjust from Try in comment 7 i.e.,
https://hg.mozilla.org/try/raw-rev/773a34e01693d8d7a0a45a22f59d5aa314cd5621
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b4c8d27ce9e
Work around clang assertion by wrapping lz4.c in an extern C block. r=froydnj
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4b4c8d27ce9e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
No longer blocks: 1398021
Regressed by: 1398021
Keywords: regression
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.