Closed Bug 1576056 Opened 4 months ago Closed 4 months ago

remove c++ standard library-detecting code from Compiler.h

Categories

(Core :: MFBT, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: shravanrn, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Building android on arm64 seems to break on try servers with some of the rlbox changes... It is not fully clear why this is the case yet.

Nathan: Any thoughts on the below?

It seems that using the std=c++17 flag on lib thebes is what triggers the build breaks with the following error messages

/builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc_abort.h:23:5: error: unknown type name 'MOZ_NORETURN'
/builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc_abort.h:25:5: error: expected unqualified-id
/builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/throw_gcc.h:51:37: error: unknown type name 'MOZ_ALWAYS_INLINE'
/builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/throw_gcc.h:51:54: error: expected unqualified-id

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=17bfcc6b4b86bb09af0141985942ab9cfd7a590d&selectedJob=263733206

Flags: needinfo?(nfroyd)

The secret here is the include stack:

...
[task 2019-08-27T18:27:21.853Z] 18:27:21     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Attributes.h:12:
[task 2019-08-27T18:27:21.853Z] 18:27:21     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Compiler.h:43:
[task 2019-08-27T18:27:21.854Z] 18:27:21     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers/cstddef:3:
[task 2019-08-27T18:27:21.854Z] 18:27:21     INFO -  In file included from /builds/worker/fetches/android-ndk/sources/cxx-stl/llvm-libc++/include/cstddef:87:
[task 2019-08-27T18:27:21.854Z] 18:27:21     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dist/stl_wrappers/type_traits:50:
[task 2019-08-27T18:27:21.854Z] 18:27:21     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:29:
[task 2019-08-27T18:27:21.854Z] 18:27:21     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dist/stl_wrappers/new:44:
[task 2019-08-27T18:27:21.854Z] 18:27:21     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers/new:3:
[task 2019-08-27T18:27:21.854Z] 18:27:21     INFO -  In file included from /builds/worker/fetches/android-ndk/sources/cxx-stl/llvm-libc++/include/new:89:
[task 2019-08-27T18:27:21.854Z] 18:27:21     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers/exception:3:
[task 2019-08-27T18:27:21.855Z] 18:27:21     INFO -  In file included from /builds/worker/fetches/android-ndk/sources/cxx-stl/llvm-libc++/include/exception:82:
[task 2019-08-27T18:27:21.855Z] 18:27:21     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dist/stl_wrappers/cstdlib:64:
[task 2019-08-27T18:27:21.855Z] 18:27:21     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/throw_gcc.h:20:
[task 2019-08-27T18:27:21.855Z] 18:27:21    ERROR -  /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc_abort.h:23:5: error: unknown type name 'MOZ_NORETURN'
[task 2019-08-27T18:27:21.855Z] 18:27:21     INFO -      MOZ_NORETURN

We're in Attributes.h -> Compiler.h, and Attributes.h is what defines MOZ_NO_RETURN. Unfortunately, we're including Compiler.h prior to the point where MOZ_NORETURN is defined, and we're winding up in a maze of system wrappers and STL headers...until we eventually arrive at mozalloc_abort.h, which wants to include Attributes.h to get at the definition of MOZ_NORETURN...but we've already included that header (or rather, we've started processing the header for inclusion, which is not quite the same thing). And so the symbol we thought was going to get defined doesn't get defined.

The rationale for including cstddef from Compiler.h is past its sell-by date, so I'll just delete that code so you can have an easier time of things.

Flags: needinfo?(nfroyd)

Thanks!
Also, feel free to change this bug's title and ownership etc. if you want to use it for the commit title

Assignee: shravanrn → nfroyd
Type: defect → task
Component: General → MFBT
OS: Android → All
Hardware: ARM64 → All
Summary: RLBox - Investigate RLBox changes that seem to break android builds → remove c++ standard library-detecting code from Compiler.h

We don't support stlport anymore, We don't use any of these macros, and
even if we did, there are probably better ways to do what we want than
by depending on the subtleties of how C++ standard libraries version
themselves.

It's not totally clear to me that the above patch will completely fix things, but ideally you should get a little farther in your adventure.

Flags: needinfo?(shravanrn)

It did get me a little further :)
Now we fail with this

builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/rlbox/rlbox_helpers.hpp:157:27: error: no template named 'invoke_result_t' in namespace 'std'

So invoke_result_t is a C++ 17 standard library function and i am passing "-std=c++17"... Also, this is a template only function, so not sure why there is an issue here...

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=62e50f2a9612b314794a57828756a6bb48311dd5&selectedJob=263766626

Flags: needinfo?(shravanrn) → needinfo?(nfroyd)

It's entirely possible that the version of libc++ that we're using on Android is too old and didn't have invoke_result_t. We can fix that.

Flags: needinfo?(nfroyd)

Sounds good. I imagine this will take some time? If so, I guess we are temporarily blocked on landing the rlbox library code (which needs c++ 17 support) until this is resolved...

Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1bd0374ac4b1
delete all the standard library-related checks in Compiler.h; r=dmajor

(In reply to Shravan Narayan from comment #8)

Sounds good. I imagine this will take some time? If so, I guess we are temporarily blocked on landing the rlbox library code (which needs c++ 17 support) until this is resolved...

It ought not to be that difficult. We just need to bump our NDK version and deal with any fallout.

(Spoiler: there is fallout from how libc++ decided to change their visibility macros: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3630beca44a1061589011a9d749b8f2dc47cf40&selectedJob=263882694)

Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
You need to log in before you can comment on or make changes to this bug.