1349064 - Review <regex> for exception safety and include it in config/stl-headers

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1349064 - Sort std-headers. -

Review commit: https://reviewboard.mozilla.org/r/122150/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122150/

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1349064 - Add <regex> to config/std-headers. -

The only exception type is regex_error, which inherits from runtime_error, which we wrap already.

Review commit: https://reviewboard.mozilla.org/r/122152/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122152/

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8849360 [details]
Bug 1349064 - Sort std-headers. -

https://reviewboard.mozilla.org/r/122150/#review124502

Clearing review because of the uncertainty noted below.

::: config/stl-headers:45
(Diff revision 1)
>  memory
>  ostream
>  set
>  stack
>  string
> +tuple

Is this commit strictly intended to sort them, or are you also requesting exception safety review on `<tuple>` as well?

(The headers are kind of sorted--external STL headers, C header wrappers, and then `xutility`, a non-standard MSVC header that needs wrapping anyway.)

Comment 4

[Kelsey Gilbert [:jgilbert]]

(In reply to Nathan Froyd [:froydnj] from comment #3)
> Comment on attachment 8849360 [details]
> Bug 1349064 - Sort std-headers. -
> 
> https://reviewboard.mozilla.org/r/122150/#review124502
> 
> Clearing review because of the uncertainty noted below.
> 
> ::: config/stl-headers:45
> (Diff revision 1)
> >  memory
> >  ostream
> >  set
> >  stack
> >  string
> > +tuple
> 
> Is this commit strictly intended to sort them, or are you also requesting
> exception safety review on `<tuple>` as well?
> 
> (The headers are kind of sorted--external STL headers, C header wrappers,
> and then `xutility`, a non-standard MSVC header that needs wrapping anyway.)

To sort them. If they are already in an order, the existence of the order should be clarified.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8849360 [details]
Bug 1349064 - Sort std-headers. -

https://reviewboard.mozilla.org/r/122150/#review124614

::: config/stl-headers:45
(Diff revision 1)
>  memory
>  ostream
>  set
>  stack
>  string
> +tuple

This only sorts.
I'm not too attached to it, but if there's no reason to sort stl/c/msvc headers separately, I'd prefer to keep it simple and just sort them as one set.

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Jeff Gilbert [:jgilbert] from comment #4)
> (In reply to Nathan Froyd [:froydnj] from comment #3)
> > ::: config/stl-headers:45
> > (Diff revision 1)
> > >  memory
> > >  ostream
> > >  set
> > >  stack
> > >  string
> > > +tuple
> > 
> > Is this commit strictly intended to sort them, or are you also requesting
> > exception safety review on `<tuple>` as well?

Ah, I see that the "moved to line X" block in mozreview hid the "tuple" reference right below it.  Sigh at too-clever review tools.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8849360 [details]
Bug 1349064 - Sort std-headers. -

https://reviewboard.mozilla.org/r/122150/#review124684

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8849360 [details]
Bug 1349064 - Sort std-headers. -

https://reviewboard.mozilla.org/r/122150/#review124936

Sorry for not noticing that `<tuple>` was already included on a previous review.

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8849361 [details]
Bug 1349064 - Add <regex> to config/std-headers. -

https://reviewboard.mozilla.org/r/122152/#review124938

::: commit-message-8bbf6:1
(Diff revision 1)
> +Bug 1349064 - Add <regex> to config/std-headers. - r=nfroydnj

Nit: it's `froydnj`, not `nfroydnj`. :)

::: commit-message-8bbf6:3
(Diff revision 1)
> +Bug 1349064 - Add <regex> to config/std-headers. - r=nfroydnj
> +
> +The only exception type is regex_error, which inherits from runtime_error, which we wrap already.

This is not a sufficient condition.  We have to ensure that whenever a `regex_error` is thrown, either via `_Xregex_error` on MSVC, `__throw_regex_error` on libstdc++, or whatever libc++ uses, that some custom function that we have gets called instead.  See memory/mozalloc/throw_gcc.h or memory/mozalloc/msvc_raise_wrappers.h.

`_Xregex_error` may require some special care in config/msvc-stl-wrapper-template.h and elsewhere, since `_Xregex_error` isn't defined in `<xutility>` like some of the other error throwers, but in `<regex>` itself -- at least it is in MSVC 2015.  I don't know if the situation is different in MSVC 2017.

Comment 10

[Kelsey Gilbert [:jgilbert]]

Here's MSVC2015:

I'm following only the !defined(_HAS_EXCEPTIONS) paths, though I think the crt is built with defined(_HAS_EXCEPTIONS).

Errors exit <regex> via this linkage:
VC/include/regex:156:
[[noreturn]] _CRTIMP2_PURE void __CLRCALL_PURE_OR_CDECL _Xregex_error(
	regex_constants::error_type _Code);



VC/crt/src/stl/xthrow.cpp:48: (with defined(_HAS_EXCEPTIONS) from when the crt is built?)
 #if _HAS_EXCEPTIONS
 #include <regex>

_STD_BEGIN
[[noreturn]] _CRTIMP2_PURE void __CLRCALL_PURE_OR_CDECL _Xregex_error(regex_constants::error_type _Code)
	{	// report a regex_error
	_THROW_NCEE(regex_error, _Code);
	}
_STD_END
 #endif /* _HAS_EXCEPTIONS */



VC/include/xstddef:78:
 #define _THROW_NCEE(x, y)	x(y)._Raise()



VC/include/exception:120: (class exception {)
	void __CLR_OR_THIS_CALL _Raise() const
		{	// raise the exception
		if (_STD _Raise_handler != 0)
			(*_STD _Raise_handler)(*this);	// call raise handler if present

		_Doraise();	// call the protected virtual
		_RAISE(*this);	// raise this exception
		}



VC/include/xstddef:65:
 #if defined(_DEBUG)
  #define _RAISE(x)	\
	_invoke_watson(_CRT_WIDE(#x), __FUNCTIONW__, __FILEW__, __LINE__, 0)

 #else /* defined(_DEBUG) */
  #define _RAISE(x)	\
	_invoke_watson(0, 0, 0, 0, 0)
 #endif /* defined(_DEBUG) */


https://msdn.microsoft.com/en-us/library/mt744305.aspx :
When the default handler _invoke_watson is called, if the processor supports a __fastfail operation, it is invoked using a parameter of FAST_FAIL_INVALID_ARG and the process terminates. Otherwise, a fast fail exception is raised, which can be caught by an attached debugger. If the process is allowed to continue, it is terminated by a call to the Windows TerminateProcess function using an exception code status of STATUS_INVALID_CRUNTIME_PARAMETER.

Of note is that MSVC has a static `exception::_Set_raise_handler(void (__cdecl *pfn)(const exception&) )`. We should consider using that instead of our #define wrapping approach.

Comment 11

[Kelsey Gilbert [:jgilbert]]

In libstdc++6.3, __throw_regex_error has two overloads:

  void
  __throw_regex_error(regex_constants::error_type __ecode);

  inline void
  __throw_regex_error(regex_constants::error_type __ecode, const char* __what)
  { _GLIBCXX_THROW_OR_ABORT(regex_error(__ecode, __what)); }


One we can hack the linkage of in throw_gcc.h (at least that's what it looks like to me?), but the second (more commonly used one) is _GLIBCXX_THROW_OR_ABORT:

#ifndef _GLIBCXX_THROW_OR_ABORT
# if __cpp_exceptions
#  define _GLIBCXX_THROW_OR_ABORT(_EXC) (throw (_EXC))
# else
#  define _GLIBCXX_THROW_OR_ABORT(_EXC) (__builtin_abort())
# endif
#endif

Since it sounds like we're aiming to crash in a particular way, we may be able to just define our own _GLIBCXX_THROW_OR_ABORT first:
#define _GLIBCXX_THROW_OR_ABORT(_EXC) (mozalloc_abort( (_EXC).what() ))

Comment 12

[(no longer active)]

This basically means that an exception aborts.  I don't understand how that makes it a safe header for use in Mozilla code?  Isn't the rule that only headers get to be listed here that return errors under error conditions?

The reason I went down the rust path in bug 1310335 was that I looked at the <regex> API and realized that the exception is baked into the interface.  :-(

Comment 13

[(no longer active)]

Bug 1310335 comment 56 has some measurements of the code size cost of the rust regex crate.  In short, that isn't an option.  So the basic answer instead of using C++11 <regex> is: use SpiderMonkey.  :/  Please note that I was working on something where I was trying to follow this advice and things weren't working out well, see bug 1350579, so if you end up going that route you should probably get some help from the JS engine folks.

Comment 14

[Kelsey Gilbert [:jgilbert]]

(In reply to :Ehsan Akhgari (busy) from comment #12)
> This basically means that an exception aborts.  I don't understand how that
> makes it a safe header for use in Mozilla code?  Isn't the rule that only
> headers get to be listed here that return errors under error conditions?

This is not my understanding. Exceptions in no-exception code should abort, both per-spec and currently in Gecko. Check the other wrappers, which are just in order to call mozalloc_abort instead of crashing directly.

There's no direct way, for instance, of making std::vector::push_back visibly fallible. That's fine, so long as we'd be using an infallible allocator anyways. If we want fallibility, we need to deal with our ns* containers.


Since <regex> isn't strongly templated, we may be able to just make a wrapper, if we did want to offer fallibility here.

Implicit to all this is the question of why we don't just selectively enable exceptions now that implementations have caught up. :)


FWIW, the only exception types that can be generated at runtime if the regex expression is fixed at compile time are:
* error_complexity: The complexity of an attempted match against a regular expression exceeded a pre-set level.
* error_stack: There was insufficient memory to determine whether the regular expression could match the specified character sequence.

Comment 15

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1349064 - Replace MSVC wrappers with std::exception::_Set_raise_handler. -

Review commit: https://reviewboard.mozilla.org/r/124402/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124402/

Comment 16

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1349064 - Wrap std::regex_error for GCC. -

Review commit: https://reviewboard.mozilla.org/r/124404/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124404/

Comment 17

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8849361 [details]
Bug 1349064 - Add <regex> to config/std-headers. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122152/diff/1-2/

Comment 18

[Nathan Froyd [:froydnj]]

Comment on attachment 8852174 [details]
Bug 1349064 - Wrap std::regex_error for GCC. -

https://reviewboard.mozilla.org/r/124404/#review127352

r+ with the change below.

::: memory/mozalloc/throw_gcc.h:35
(Diff revision 1)
> +// Handle `_GLIBCXX_THROW_OR_ABORT(regex_error(__ecode, __what));` et al.
> +#define _GLIBCXX_THROW_OR_ABORT(_EXC) mozalloc_abort( (_EXC).what() )

I have a hard time believing that this isn't going to break other things if `_GLIBCXX_THROW_OR_ABORT` is used with non-`regex_error` exceptions.  And I think that since we compile with `-fno-exceptions` anyway, the macro will wind up `abort()`'ing, which is suboptimal but OK.  Please remove this.

I see that GCC 6 seems to have added this to `<regex>` rather than doing things the proper way...sigh.  Maybe there were ABI constraints in play.

Comment 19

[Nathan Froyd [:froydnj]]

(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> (In reply to :Ehsan Akhgari (busy) from comment #12)
> > This basically means that an exception aborts.  I don't understand how that
> > makes it a safe header for use in Mozilla code?  Isn't the rule that only
> > headers get to be listed here that return errors under error conditions?
> 
> This is not my understanding. Exceptions in no-exception code should abort,
> both per-spec and currently in Gecko. Check the other wrappers, which are
> just in order to call mozalloc_abort instead of crashing directly.

This is correct: the condition for something being included in stl-headers is that somebody has looked at all the places where things can throw in that header and determined that we wrap them appropriately such that mozalloc_abort is called instead.

> Implicit to all this is the question of why we don't just selectively enable
> exceptions now that implementations have caught up. :)

What does "selectively enable" mean?  Individual directories?  No thank you.

The last time I remember hearing a discussion about it, there were several concerns:

* Binary size;
* Optimization inhibition;
* Exception safety of existing and new code.

Comment 20

[Nathan Froyd [:froydnj]]

Comment on attachment 8849361 [details]
Bug 1349064 - Add <regex> to config/std-headers. -

https://reviewboard.mozilla.org/r/122152/#review127356

r=me, subject to the other patches in this bug being approved.

::: commit-message-8bbf6:3
(Diff revision 2)
> +Bug 1349064 - Add <regex> to config/std-headers. - r=froydnj
> +
> +The only exception type is regex_error, which inherits from runtime_error, which we wrap already.

Please delete this bit of the commit message, since it's not accurate.

Comment 21

[Nathan Froyd [:froydnj]]

Comment on attachment 8852173 [details]
Bug 1349064 - Replace MSVC wrappers with std::exception::_Set_raise_handler. -

https://reviewboard.mozilla.org/r/124402/#review127358

I *think* this is OK?  Requesting glandium's review because he knows more about potential pitfalls here than I do.

I'm not super-excited about using something I can't even find documentation for on MSDN...

::: memory/mozalloc/moz.build:21
(Diff revision 1)
> -            EXPORTS.mozilla += [
> -                'msvc_raise_wrappers.h',
> -                'throw_msvc.h',
> -            ]
>              SOURCES += [
>                  'msvc_raise_wrappers.cpp',

Presumably we'd want to call this something else?

::: memory/mozalloc/msvc_raise_wrappers.cpp:18
(Diff revision 1)
> -moz_Xruntime_error(const char* what)
> -{
> +    Foo() {
> +        std::exception::_Set_raise_handler(RaiseHandler);
> -    abort_from_exception("runtime_error", what);
> -}
> +    }

I am not 100% convinced this is safe: having this be a static constructor means that its execution order with respect to other constructors is undefined, which means that our raise handler may not be installed when exceptions get thrown from other static constructors.

OTOH, if somebody is throwing from a static constructor, we're already in trouble, so...

Comment 22

[Nathan Froyd [:froydnj]]

Comment on attachment 8852173 [details]
Bug 1349064 - Replace MSVC wrappers with std::exception::_Set_raise_handler. -

Sigh at mozreview changes not being propagated back to bugzilla flags.

Comment 23

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj] from comment #19)
> The last time I remember hearing a discussion about it, there were several
> concerns:
> 
> * Binary size;
> * Optimization inhibition;
> * Exception safety of existing and new code.

For the record, this is the discussion I was thinking of:

https://groups.google.com/d/msg/mozilla.dev.tech.js-engine.internals/xz4LLslf7ZM/vv_pdO1DBAAJ

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 8852173 [details]
Bug 1349064 - Replace MSVC wrappers with std::exception::_Set_raise_handler. -

https://reviewboard.mozilla.org/r/124402/#review127358

> I am not 100% convinced this is safe: having this be a static constructor means that its execution order with respect to other constructors is undefined, which means that our raise handler may not be installed when exceptions get thrown from other static constructors.
> 
> OTOH, if somebody is throwing from a static constructor, we're already in trouble, so...

Additionally, this is in libmozglue, which is loaded before everything else in gecko, so there are less static constructors possibly running before this. And probably none of them can ever throw exceptions (I think the rare things in mozglue that have static constructors on Windows are all in C).

Comment 25

[Mike Hommey [:glandium]]

Comment on attachment 8852173 [details]
Bug 1349064 - Replace MSVC wrappers with std::exception::_Set_raise_handler. -

https://reviewboard.mozilla.org/r/124402/#review128820

::: memory/mozalloc/msvc_raise_wrappers.cpp:17
(Diff revision 1)
> -MFBT_API __declspec(noreturn) void
> -moz_Xruntime_error(const char* what)
> -{
> +static struct Foo {
> +    Foo() {
> +        std::exception::_Set_raise_handler(RaiseHandler);
> -    abort_from_exception("runtime_error", what);
> -}
> +    }
> -
> +} Bar;

Something more descriptive than "Foo" and "Bar" would be great.

Comment 26

[Kelsey Gilbert [:jgilbert]]

(In reply to Nathan Froyd [:froydnj] from comment #18)
> Comment on attachment 8852174 [details]
> Bug 1349064 - Wrap std::regex_error for GCC. -
> 
> https://reviewboard.mozilla.org/r/124404/#review127352
> 
> r+ with the change below.
> 
> ::: memory/mozalloc/throw_gcc.h:35
> (Diff revision 1)
> > +// Handle `_GLIBCXX_THROW_OR_ABORT(regex_error(__ecode, __what));` et al.
> > +#define _GLIBCXX_THROW_OR_ABORT(_EXC) mozalloc_abort( (_EXC).what() )
> 
> I have a hard time believing that this isn't going to break other things if
> `_GLIBCXX_THROW_OR_ABORT` is used with non-`regex_error` exceptions.  And I
> think that since we compile with `-fno-exceptions` anyway, the macro will
> wind up `abort()`'ing, which is suboptimal but OK.  Please remove this.
> 
> I see that GCC 6 seems to have added this to `<regex>` rather than doing
> things the proper way...sigh.  Maybe there were ABI constraints in play.

I'm concerned that if we don't override _GLIBCXX_THROW_OR_ABORT, we'll get aborts which don't give us crash reports. Are you sure you don't want this overwrite?

I'll land this without for now. I do want to try switching to catching SIGABRT with signal() anyways.

Comment 27

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8849361 [details]
Bug 1349064 - Add <regex> to config/std-headers. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122152/diff/2-3/

Comment 28

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8852173 [details]
Bug 1349064 - Replace MSVC wrappers with std::exception::_Set_raise_handler. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124402/diff/1-2/

Comment 29

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8852174 [details]
Bug 1349064 - Wrap std::regex_error for GCC. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124404/diff/1-2/

Comment 30

[Nathan Froyd [:froydnj]]

Comment on attachment 8852173 [details]
Bug 1349064 - Replace MSVC wrappers with std::exception::_Set_raise_handler. -

https://reviewboard.mozilla.org/r/124402/#review129076

Comment 31

[Nathan Froyd [:froydnj]]

(In reply to Jeff Gilbert [:jgilbert] from comment #26)
> I'm concerned that if we don't override _GLIBCXX_THROW_OR_ABORT, we'll get
> aborts which don't give us crash reports. Are you sure you don't want this
> overwrite?

My concern here is that if you do override _GLIBCXX_THROW_OR_ABORT and it gets invoked with a non-<regex> error, you can't compile Firefox at all because the compiler will complain about a non-existent field reference.  This doesn't affect our release builds at this point, because _GLIBCXX_THROW_OR_ABORT doesn't get used with the problematic regex_error instances in the version of GCC we use to compile Firefox.

In any event, we already have this problem because std::__throw_bad_weak_ptr uses _GLIBCXX_THROW_OR_ABORT...

This sounds sane, right glandium?

Comment 32

[Mike Hommey [:glandium]]

We're going through lengths here and there to avoid abort() and have things go through e.g. mozalloc_abort(). IIRC that's because the latter is guaranteed to trigger the crash reporter.

Now, as for the risk with _GLIBCXX_THROW_OR_ABORT being invoked with a non-<regex> error, _GLIBCXX_THROW_OR_ABORT is actually just `throw` when exceptions are enabled. Which means it's always given a std::exception, and the .what() in the patch's override for _GLIBCXX_THROW_OR_ABORT is actually a member of std::exception (inherited by regex_error through runtime_error), so I don't think there's any risk of it breaking anything.

So it feels to me we're actually in a better place if we override it.

Does it make sense?

Comment 33

[Nathan Froyd [:froydnj]]

Oh, indeed!  For some reason, I thought what() was a regex_error-only API, but looking back at the code now, I can't see why I thought that.  My mistake.

Let's keep the _GLIBCXX_THROW_OR_ABORT definition.  Thanks for working through this.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc695c2ea24e
Sort std-headers. - r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/02e10c8eeb95
Add <regex> to config/std-headers. - r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c35a43033bc
Replace MSVC wrappers with std::exception::_Set_raise_handler. - r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/1506a6e5ee37
Wrap std::regex_error for GCC. - r=glandium

Comment 35

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for breaking Android build:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7222624828398550e7d4964a6d91ec92e89aa517
https://hg.mozilla.org/integration/mozilla-inbound/rev/e38daced6d84898d47541ea2bb2ca741b0a77660
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd2f9fee24e6487e59cbc10adb2658bb5d39ddee
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f6eed6971d566b05b263109b15864c187cb4d94

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1506a6e5ee3711cb8690f35aac5a95b17458214e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=89280252&repo=mozilla-inbound

[task 2017-04-06T20:25:18.563265Z] 20:25:18     INFO -  /home/worker/workspace/build/src/android-ndk/sources/cxx-stl/llvm-libc++/libcxx/include/functional:1603:72:   required from 'std::__ndk1::function<_Rp(_ArgTypes ...)>::function(_Fp, typename std::__ndk1::enable_if<(std::__ndk1::function<_Rp(_ArgTypes ...)>::__callable<_Fp>::value && (! std::__ndk1::is_same<_Fp, std::__ndk1::function<_Rp(_ArgTypes ...)> >::value))>::type*) [with _Fp = GrBatchFlushState::doUpload(GrDrawBatch::DeferredUploadFn&)::<lambda(GrSurface*, int, int, int, int, GrPixelConfig, const void*, size_t)>; _Rp = bool; _ArgTypes = {GrSurface*, int, int, int, int, GrPixelConfig, const void*, unsigned int}; typename std::__ndk1::enable_if<(std::__ndk1::function<_Rp(_ArgTypes ...)>::__callable<_Fp>::value && (! std::__ndk1::is_same<_Fp, std::__ndk1::function<_Rp(_ArgTypes ...)> >::value))>::type = void]'
[task 2017-04-06T20:25:18.563334Z] 20:25:18     INFO -  /home/worker/workspace/build/src/gfx/skia/skia/src/gpu/GrBatchFlushState.h:80:9:   required from here
[task 2017-04-06T20:25:18.563901Z] 20:25:18     INFO -  /home/worker/workspace/build/src/android-ndk/sources/cxx-stl/llvm-libc++/libcxx/include/memory:2630:34: error: invalid conversion from 'std::__ndk1::unique_ptr<std::__ndk1::__function::__base<bool(GrSurface*, int, int, int, int, GrPixelConfig, const void*, unsigned int)>, std::__ndk1::__allocator_destructor<std::__ndk1::allocator<std::__ndk1::__function::__func<GrBatchFlushState::doUpload(GrDrawBatch::DeferredUploadFn&)::<lambda(GrSurface*, int, int, int, int, GrPixelConfig, const void*, size_t)>, std::__ndk1::allocator<GrBatchFlushState::doUpload(GrDrawBatch::DeferredUploadFn&)::<lambda(GrSurface*, int, int, int, int, GrPixelConfig, const void*, size_t)> >, bool(GrSurface*, int, int, int, int, GrPixelConfig, const void*, unsigned int)> > > >::pointer {aka std::__ndk1::__function::__base<bool(GrSurface*, int, int, int, int, GrPixelConfig, const void*, unsigned int)>*}' to 'std::__ndk1::__allocator_destructor<std::__ndk1::allocator<std::__ndk1::__function::__func<GrBatchFlushState::doUpload(GrDrawBatch::DeferredUploadFn&)::<lambda(GrSurface*, int, int, int, int, GrPixelConfig, const void*, size_t)>, std::__ndk1::allocator<GrBatchFlushState::doUpload(GrDrawBatch::DeferredUploadFn&)::<lambda(GrSurface*, int, int, int, int, GrPixelConfig, const void*, size_t)> >, bool(GrSurface*, int, int, int, int, GrPixelConfig, const void*, unsigned int)> > >::pointer {aka std::__ndk1::__function::__func<GrBatchFlushState::doUpload(GrDrawBatch::DeferredUploadFn&)::<lambda(GrSurface*, int, int, int, int, GrPixelConfig, const void*, size_t)>, std::__ndk1::allocator<GrBatchFlushState::doUpload(GrDrawBatch::DeferredUploadFn&)::<lambda(GrSurface*, int, int, int, int, GrPixelConfig, const void*, size_t)> >, bool(GrSurface*, int, int, int, int, GrPixelConfig, const void*, unsigned int)>*}' [-fpermissive]
[task 2017-04-06T20:25:18.570171Z] 20:25:18     INFO -               __ptr_.second()(__tmp);
[task 2017-04-06T20:25:18.570227Z] 20:25:18     INFO -                                    ^
[task 2017-04-06T20:25:18.570288Z] 20:25:18     INFO -  In file included from /home/worker/workspace/build/src/obj-firefox/dist/system_wrappers/memory:3:0,
[task 2017-04-06T20:25:18.570347Z] 20:25:18     INFO -                   from /home/worker/workspace/build/src/obj-firefox/dist/stl_wrappers/memory:44,
[task 2017-04-06T20:25:18.570410Z] 20:25:18     INFO -                   from /home/worker/workspace/build/src/android-ndk/sources/cxx-stl/llvm-libc++/libcxx/include/functional:477,
[task 2017-04-06T20:25:18.570468Z] 20:25:18     INFO -                   from /home/worker/workspace/build/src/obj-firefox/dist/system_wrappers/functional:3,
[task 2017-04-06T20:25:18.570523Z] 20:25:18     INFO -                   from /home/worker/workspace/build/src/obj-firefox/dist/stl_wrappers/functional:44,
[task 2017-04-06T20:25:18.570579Z] 20:25:18     INFO -                   from /home/worker/workspace/build/src/gfx/skia/skia/include/core/SkRefCnt.h:14,
[task 2017-04-06T20:25:18.570637Z] 20:25:18     INFO -                   from /home/worker/workspace/build/src/gfx/skia/skia/include/gpu/GrColorSpaceXform.h:13,
[task 2017-04-06T20:25:18.570700Z] 20:25:18     INFO -                   from /home/worker/workspace/build/src/gfx/skia/skia/include/gpu/GrTestUtils.h:16,
[task 2017-04-06T20:25:18.570762Z] 20:25:18     INFO -                   from /home/worker/workspace/build/src/gfx/skia/skia/src/gpu/GrBatchTest.h:11,
[task 2017-04-06T20:25:18.570831Z] 20:25:18     INFO -                   from /home/worker/workspace/build/src/gfx/skia/skia/src/gpu/GrDrawContext.cpp:8:
[task 2017-04-06T20:25:18.571214Z] 20:25:18     INFO -  /home/worker/workspace/build/src/android-ndk/sources/cxx-stl/llvm-libc++/libcxx/include/memory:3474:10: note: initializing argument 1 of 'void std::__ndk1::__allocator_destructor<_Alloc>::operator()(std::__ndk1::__allocator_destructor<_Alloc>::pointer) [with _Alloc = std::__ndk1::allocator<std::__ndk1::__function::__func<GrBatchFlushState::doUpload(GrDrawBatch::DeferredUploadFn&)::<lambda(GrSurface*, int, int, int, int, GrPixelConfig, const void*, size_t)>, std::__ndk1::allocator<GrBatchFlushState::doUpload(GrDrawBatch::DeferredUploadFn&)::<lambda(GrSurface*, int, int, int, int, GrPixelConfig, const void*, size_t)> >, bool(GrSurface*, int, int, int, int, GrPixelConfig, const void*, unsigned int)> >; std::__ndk1::__allocator_destructor<_Alloc>::pointer = std::__ndk1::__function::__func<GrBatchFlushState::doUpload(GrDrawBatch::DeferredUploadFn&)::<lambda(GrSurface*, int, int, int, int, GrPixelConfig, const void*, size_t)>, std::__ndk1::allocator<GrBatchFlushState::doUpload(GrDrawBatch::DeferredUploadFn&)::<lambda(GrSurface*, int, int, int, int, GrPixelConfig, const void*, size_t)> >, bool(GrSurface*, int, int, int, int, GrPixelConfig, const void*, unsigned int)>*]'
[task 2017-04-06T20:25:18.571268Z] 20:25:18     INFO -       void operator()(pointer __p) _NOEXCEPT
[task 2017-04-06T20:25:18.571302Z] 20:25:18     INFO -            ^
[task 2017-04-06T20:25:18.571365Z] 20:25:18     INFO -  /home/worker/workspace/build/src/config/rules.mk:986: recipe for target 'GrDrawContext.o' failed
[task 2017-04-06T20:25:18.571570Z] 20:25:18     INFO -  gmake[5]: *** [GrDrawContext.o] Error 1
[task 2017-04-06T20:25:18.571912Z] 20:25:18     INFO -  gmake[5]: Leaving directory '/home/worker/workspace/build/src/obj-firefox/gfx/skia'
[task 2017-04-06T20:25:18.572274Z] 20:25:18     INFO -  /home/worker/workspace/build/src/config/recurse.mk:73: recipe for target 'gfx/skia/target' failed
[task 2017-04-06T20:25:18.574860Z] 20:25:18     INFO -  gmake[4]: *** [gfx/skia/target] Error 2

Comment 36

[Mike Hommey [:glandium]]

Ah, we're using libc++ on android, aren't we?

Comment 37

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #36)
> Ah, we're using libc++ on android, aren't we?

Indeed.

Comment 38

[Bruce Sun [:brsun]]

Hi Jeff,

Your patches could solve several build errors while including MSVC STL headers (i.e. utility) on Windows[1][2]. Do we have the luck to see MSVC specific part being fixed recently?

[1] bug 1350261 (this bug should be reopened if DISABLE_STL_WRAPPING is not recommended)
[2] bug 1368948 comment 56

Comment 39

[Mike Hommey [:glandium]]

The only reason the patch series in this bug was backed out is Android. The MSVC changes from https://hg.mozilla.org/integration/mozilla-inbound/rev/8c35a43033bc do not affect that. So that changeset can be relanded.

Comment 40

[Astley Chen (inactive)]

(In reply to Mike Hommey [:glandium] from comment #39)
> The only reason the patch series in this bug was backed out is Android. The
> MSVC changes from
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8c35a43033bc do not
> affect that. So that changeset can be relanded.

Make sense to me. In order to not block bug 1368948, we could probably land this patch first.
Jeff(:jgilbert), do you have any concern? Or Bruce could help to do that if you are busy on something esle.

Comment 41

[Bruce Sun [:brsun]]

The build results on the try server[1] with the latest m-c seem good.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dbb98c41fedb7940ecd4169e972ef4a3ccf08cc

Comment 42

[Kelsey Gilbert [:jgilbert]]

By all means, let's land the separable part.

Comment 43

[Kelsey Gilbert [:jgilbert]]

Let's split this off into another bug, though.

Comment 44

[Bruce Sun [:brsun]]

Hi Jeff, bug 1376057 has just been created. Would you like to use that bug to land your patch? Feel free to change the title if it is not precise enough.

Comment 45

[Kelsey Gilbert [:jgilbert]]

(In reply to Bruce Sun [:brsun] from comment #44)
> Hi Jeff, bug 1376057 has just been created. Would you like to use that bug
> to land your patch? Feel free to change the title if it is not precise
> enough.

Looks good. Let's land it there.

Comment 46

[Kelsey Gilbert [:jgilbert]]

I never went back and landed these, so let's do that.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=801c033b60bc1d62f41e0610b70fe3c1609c83a1

[task 2019-02-20T23:07:19.925Z] 23:07:19     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/testing/tools/screenshot'
[task 2019-02-20T23:07:19.925Z] 23:07:19     INFO -  /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/clang/bin/x86_64-w64-mingw32-clang++ -mwindows -o ../../../dist/bin/screenshot.exe -Qunused-arguments -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++1z-compat -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=return-std-move -Wno-error=atomic-alignment -Wno-unknown-pragmas -Wno-unused-function -Wno-conversion-null -Wno-switch -Wno-enum-compare -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -Wno-return-type-c-linkage -fno-sized-deallocation -fms-extensions -fno-exceptions -fno-strict-aliasing -mms-bitfields -Wno-incompatible-ms-struct -fno-rtti -ffunction-sections -fdata-sections -Wa,-mbig-obj -fno-exceptions -fno-math-errno -pipe -g -gcodeview -fno-omit-frame-pointer -funwind-tables  win32-screenshot.o ./module.res -municode -mconsole  -Wl,-S -Wl,--dynamicbase -Wl,--icf=safe     -pie -Wl,-pdb,../../../dist/bin//screenshot.pdb  -luuid -lusp10 -lgdi32 -lwinmm -lwsock32 -luserenv -lsecur32 -lgdiplus
[task 2019-02-20T23:07:19.925Z] 23:07:19     INFO -  lld-link: error: undefined symbol: __imp_mozalloc_abort
[task 2019-02-20T23:07:19.925Z] 23:07:19     INFO -  >>> referenced by /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/throw_gcc.h:140
[task 2019-02-20T23:07:19.925Z] 23:07:19     INFO -  >>>               win32-screenshot.o:(_ZSt19__throw_regex_errori)
[task 2019-02-20T23:07:19.925Z] 23:07:19     INFO -  clang-8: error: linker command failed with exit code 1 (use -v to see invocation)
[task 2019-02-20T23:07:19.926Z] 23:07:19     INFO -  /builds/worker/workspace/build/src/config/rules.mk:532: recipe for target '../../../dist/bin/screenshot.exe' failed
[task 2019-02-20T23:07:19.926Z] 23:07:19    ERROR -  make[4]: *** [../../../dist/bin/screenshot.exe] Error 1

Any ideas, glandium?

Comment 47

[Mike Hommey [:glandium]]

Add DisableStlWrapping() to testing/tools/screenshot/moz.build.

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb5b654eaae7
Sort std-headers. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f821a2db6c5
Add <regex> to config/std-headers. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/37d17cc8cce5
Wrap std::regex_error for GCC. r=glandium

Comment 49

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/eb5b654eaae7
https://hg.mozilla.org/mozilla-central/rev/6f821a2db6c5
https://hg.mozilla.org/mozilla-central/rev/37d17cc8cce5