Closed Bug 1164522 Opened 9 years ago Closed 8 years ago

What type should we use for lambdas?

Categories

(Core :: MFBT, defect)

Product:
Core
Component:
MFBT
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1198451

People

(Reporter: nbp, Unassigned)

References

(Blocks 1 open bug)

Details

Currently we are unable to express lambda as argument of a function. The usual way is to use std::function<TYPE> for arguments, but using this for any argument add the following symbol

                 U _ZSt25__throw_bad_function_callv

We need a type which is equivalent to  std::function  and which has no risk of making exception.

Currently the solution used for taking lambdas as argument is to use a template, and let the function be specialized for each lambda.  This implies that the template definition has to be visible to all uses, as we cannot do any pre-declaration for lambda types.

Another solution which works for arguments, and for returning lambda is to use function pointers, but this enforce that the lambda has no context.

Currently, there is no solution which allow us to keep the declaration out-side of headers (without template) and which allow returning a lambda with copied-context (because you don't want to be taking references).

I noticed this issue with Bug 1147403 part 0.
(In reply to Nicolas B. Pierron [:nbp] from comment #0)
> Currently we are unable to express lambda as argument of a function. The
> usual way is to use std::function<TYPE> for arguments, but using this for
> any argument add the following symbol
> 
>                  U _ZSt25__throw_bad_function_callv

Where is that coming from?  Can we fix it?
[1] http://en.cppreference.com/w/cpp/utility/functional/function
“The stored callable object is called the target of std::function. If a std::function contains no target, it is called empty. Invoking the target of an empty std::function results in std::bad_function_call exception being thrown.”

So, I guess we would probably want to have MOZ_RELEASE_ASSERT or just to have no option for creating an empty function.
(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> So, I guess we would probably want to have MOZ_RELEASE_ASSERT or just to
> have no option for creating an empty function.

Since std::function supports function pointers, if we want to retain compatibility it's not possible to statically prevent creation of an empty function.
We should really figure out a solution here - this seems to keep getting in my way when I try to use lambdas.
(In reply to Seth Fowler [:seth] from comment #3)
> (In reply to Nicolas B. Pierron [:nbp] from comment #2)
> > So, I guess we would probably want to have MOZ_RELEASE_ASSERT or just to
> > have no option for creating an empty function.
> 
> Since std::function supports function pointers, if we want to retain
> compatibility it's not possible to statically prevent creation of an empty
> function.

Can we make something like std::function which has a default catch-all function which result in a MOZ_CRASH if not initialized with a function yet?
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #1)
> (In reply to Nicolas B. Pierron [:nbp] from comment #0)
> > Currently we are unable to express lambda as argument of a function. The
> > usual way is to use std::function<TYPE> for arguments, but using this for
> > any argument add the following symbol
> > 
> >                  U _ZSt25__throw_bad_function_callv
> 
> Where is that coming from?  Can we fix it?
Flags: needinfo?(nicolas.b.pierron)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #1)
> (In reply to Nicolas B. Pierron [:nbp] from comment #0)
> > 
> >                  U _ZSt25__throw_bad_function_callv
> 
> Where is that coming from?  Can we fix it?

This is me using "nm" on the binary of the JS shell, after using std::function as argument of a method. I even wonder how it managed to produce a binary with unknown symbols.
Flags: needinfo?(nicolas.b.pierron)
What C++ library are you using?  Do we pass in the correct -l flags and whatnot to the compiler?  My point was, this might just be a bug in the build system or some such
It's a symbol in libstdc++

$ objdump -T /usr/lib/x86_64-linux-gnu/libstdc++.so.6|grep _ZSt25__throw_bad_function_callv
000000000008ace0 g    DF .text	0000000000000032  GLIBCXX_3.4.14 _ZSt25__throw_bad_function_callv

That doesn't make it something we want, though.
In bug 1198451, I've added a mozilla::Function class to MFBT which is similar in purpose to std::function. Does that fill the need described in this bug?
Flags: needinfo?(nicolas.b.pierron)
(In reply to Botond Ballo [:botond] from comment #10)
> In bug 1198451, I've added a mozilla::Function class to MFBT which is
> similar in purpose to std::function. Does that fill the need described in
> this bug?

It tihnk the current implementation of mozilla::Function lacks check for OOM (from the caller of the mozilla::Function), and I would prefer to avoid creating copies of the lambda in favor of moving the lambda instead.
Flags: needinfo?(nicolas.b.pierron)
Sorry, I overlooked this comment...

(In reply to Nicolas B. Pierron [:nbp] from comment #11)
> It tihnk the current implementation of mozilla::Function lacks check for OOM
> (from the caller of the mozilla::Function)

I don't understand this part. Who should be checking for OOM and when?

> and I would prefer to avoid
> creating copies of the lambda in favor of moving the lambda instead.

That's a good point; we should support moving the lambda into the mozilla::function.
(In reply to Botond Ballo [:botond] from comment #12)
> Sorry, I overlooked this comment...
> 
> (In reply to Nicolas B. Pierron [:nbp] from comment #11)
> > It tihnk the current implementation of mozilla::Function lacks check for OOM
> > (from the caller of the mozilla::Function)
> 
> I don't understand this part. Who should be checking for OOM and when?

Sorry, my bad, the code does check for OOM.  Still I think this is one of the defect of the current interface, which is that one has to check that the callback allocation might fail, and allocating in the constructor does not make this obvious.

Taking a random use [0,1,2,3] of the mozilla::function class highlight that one developer forgot to check for OOM when using this class.  I honestly do not blame the developer for making such mistake, as this interface is too close to std::function, and thus does not enforce checks for OOMs.

Thus, I think we should not be surprised if we have issues [4] in case of allocation failures.

> > and I would prefer to avoid
> > creating copies of the lambda in favor of moving the lambda instead.
> 
> That's a good point; we should support moving the lambda into the
> mozilla::function.

I think we should diverge from the std::function implementation and make RAII reference by default, and make it explicit that the developer has to allocate a placeholder iff we still have a non-null ref-counter to the stack reference FunctionImpl.

This might also give a hint on the fact that the lambda is allowed reference the stack or not.  If we were to enforce that the allocation happens in the same function as the RAII, then we could have a clang analysis to prevent dangling stack references from lambdas.

[0] http://searchfox.org/mozilla-central/source/gfx/layers/apz/util/APZEventState.h#34
[1] http://searchfox.org/mozilla-central/source/dom/ipc/TabChild.cpp#870
[2] http://searchfox.org/mozilla-central/source/gfx/layers/apz/util/APZEventState.h#45,81
[3] http://searchfox.org/mozilla-central/search?q=symbol:F_%3CT_mozilla%3A%3Alayers%3A%3AAPZEventState%3E_4&redirect=false
[4] http://searchfox.org/mozilla-central/source/mfbt/Function.h#143,162,179-180
Still, I guess this bug is solved, and the answer is mozilla::function.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
(In reply to Nicolas B. Pierron [:nbp] from comment #13)
> Taking a random use [0,1,2,3] of the mozilla::function class highlight that
> one developer forgot to check for OOM when using this class.  I honestly do
> not blame the developer for making such mistake, as this interface is too
> close to std::function, and thus does not enforce checks for OOMs.
> 
> Thus, I think we should not be surprised if we have issues [4] in case of
> allocation failures.

I was the one who wrote that particular use of mozilla::function :)

You are right that I did not check for allocation failure - but my understanding is that's something we don't check in general. For some other examples unrelated to mozilla::function, see [0, 1, 2, 3, 4]. 

[0] http://searchfox.org/mozilla-central/rev/bfcc10319e4e3ce78367fa9bba9316f7eb5248b6/gfx/layers/apz/src/AsyncPanZoomController.cpp#669
[1] http://searchfox.org/mozilla-central/rev/bfcc10319e4e3ce78367fa9bba9316f7eb5248b6/layout/base/FrameLayerBuilder.cpp#143
[2] http://searchfox.org/mozilla-central/rev/bfcc10319e4e3ce78367fa9bba9316f7eb5248b6/layout/base/nsDisplayList.cpp#1746
[3] http://searchfox.org/mozilla-central/rev/bfcc10319e4e3ce78367fa9bba9316f7eb5248b6/gfx/layers/Layers.cpp#306
[4] http://searchfox.org/mozilla-central/rev/bfcc10319e4e3ce78367fa9bba9316f7eb5248b6/layout/base/nsCSSFrameConstructor.cpp#9879

If we are supposed to be checking for allocation failure in all those places, we have a much bigger problem!

> I think we should diverge from the std::function implementation and make
> RAII reference by default, and make it explicit that the developer has to
> allocate a placeholder iff we still have a non-null ref-counter to the stack
> reference FunctionImpl.
> 
> This might also give a hint on the fact that the lambda is allowed reference
> the stack or not.  If we were to enforce that the allocation happens in the
> same function as the RAII, then we could have a clang analysis to prevent
> dangling stack references from lambdas.

I'm not sure I understand this. Are you saying mozilla::function should hold a reference to the callable objected which is allocated elsewhere by the caller?
(In reply to Botond Ballo [:botond] from comment #15)
> If we are supposed to be checking for allocation failure in all those
> places, we have a much bigger problem!

I would expect that to be the case in the JS engine.

> (In reply to Nicolas B. Pierron [:nbp] from comment #13)
> > I think we should diverge from the std::function implementation and make
> > RAII reference by default, and make it explicit that the developer has to
> > allocate a placeholder iff we still have a non-null ref-counter to the stack
> > reference FunctionImpl.
> > 
> > This might also give a hint on the fact that the lambda is allowed reference
> > the stack or not.  If we were to enforce that the allocation happens in the
> > same function as the RAII, then we could have a clang analysis to prevent
> > dangling stack references from lambdas.
> 
> I'm not sure I understand this. Are you saying mozilla::function should hold
> a reference to the callable objected which is allocated elsewhere by the
> caller?

Yes, I think it would be nice to have something similar to:

void foo(mozilla:function f);

void bar()
{
  char a;
  foo(StackFunction([&] () { return a; }))
}

bool baz()
{
  char a;
  auto f = HeapFunction([&] () { return a; }); // (static analysis) ERROR: uses stack reference in
                                               // heap-allocated functions.
  if (!f)
    return false
  auto g = HeapFunction([=] () { return a; });
  if (!g)
    return false
  foo(g);
  return true;
}

Also, I would expect the StackFunction to assert if we still have references to it after the end of its scope.

In your cases, you would probably have a HeapFunction, with no OOM check in the caller if this is what is expected, but this is definitely something that I would want to see as being explicit rather than implicit, especially if we can avoid the lambda pitfall of use-after-free stack references.
How would you avoid the referred-to function object going out of scope?

mozilla::function f;
if (foo) {
  f = StackFunction([]() { ... });
}
f();  // oops, the lambda has gone out of scope!
(In reply to Botond Ballo [:botond] from comment #17)
> How would you avoid the referred-to function object going out of scope?
> 
> mozilla::function f;
> if (foo) {
>   f = StackFunction([]() { ... });
> }
> f();  // oops, the lambda has gone out of scope!

Unfortunately, not statically, but dynamically by making the StackFunction a MOZ_STACK_CLASS, which has a ref-counted stack-allocated FunctionImpl and which asserts that the ref-count reaches zero when the StackFunction is destroyed.
(In reply to Nicolas B. Pierron [:nbp] from comment #18)
> (In reply to Botond Ballo [:botond] from comment #17)
> > How would you avoid the referred-to function object going out of scope?
> > 
> > mozilla::function f;
> > if (foo) {
> >   f = StackFunction([]() { ... });
> > }
> > f();  // oops, the lambda has gone out of scope!
> 
> Unfortunately, not statically, but dynamically by making the StackFunction a
> MOZ_STACK_CLASS, which has a ref-counted stack-allocated FunctionImpl and
> which asserts that the ref-count reaches zero when the StackFunction is
> destroyed.

Ok, I see. That's an interesting idea!

Feel free to file an MFBT bug suggesting this as an enhancement. I'm not sure what our plans are wrt. keeping mozilla::function or replacing it with std::function, but it would be good to have this on file so we can keep it in mind if the question comes up.
Blocks: 1294408
You need to log in before you can comment on or make changes to this bug.