Re: Hiding 'new' statements - Good or Evil?

[Jeff Gilbert]

I would say it comes down to the module. I'm very comfortable in my
modules using auto, perhaps because our lifetime management is more
clear. To me, the unsafe examples are pretty strawman-y, since it's
very clear that there are at-risk lifetimes involved. (Returning a
bare pointer and relying on it staying valid is bad; const auto& takes
a reference, not a strong reference. If you want a new strong
reference, you obviously can't just use a reference)

`auto` was formalized in c++11, and it is now 2018. C++11 has been
required to compile Firefox since Firefox 25 in mid 2013. The rules
for auto are pretty easy to understand, too. (strips the c/v/r
decorations from the returning type, so that `const int& bar(); auto
foo = bar();` gives `foo` type `int`) In my mind, pushing back on auto
here is effectively a statement that we never expect our reviewing
ability to expand to include any new constructs, because we might
potentially maybe use those new constructs wrong. Yes, we should
always carefully consider new things. No, we shouldn't use every
new-fangled bit of the language. But honestly if you're writing c++
without auto today, it's not modern c++.

If you think auto isn't right for your module, don't accept it. That
much is agreeable, I think. To my knowledge, we've been using it for
years and so far I've only run into one at-all-related issue with it,
which ended up being a miscompilation on gcc<7 that affects all
references (including non-auto) to member scalars.
(https://bugzilla.mozilla.org/show_bug.cgi?id=1329044)

On Thu, Jan 4, 2018 at 11:31 AM, Randell Jesup  wrote:
> [SNIP]
>>> If foo->bar() can mutate the lifetime of foo, of course you must take a 
>>> strong
>>> reference to foo. Nothing about auto or range-for changes this.
>>
>>What auto does is make it really hard to tell whether a strong reference is
>>being taken.
>>
>>> If you don't understand your lifetimes, you get bugs.
>>
>>Fully agreed.  The discussion is about whether auto helps or hinders that
>>understanding.  The answer is that it depends on the surrounding code, from
>>where I sit...
>>
>>-Boris
>
> So, where do we go from here?
>
> It's clear that auto is not as safe as some/many believe it to be; it
> can as Boris (and smaug and Xidorn) say hide lifetime issues and lead to
> non-obvious UAFs and the like.  Some uses are certainly safe, but it
> isn't always obvious looking at a patch (per above), requiring more code
> investigation by the reviewer.  If the resultant type isn't obvious,
> then using it isn't helping comprehension.  When reading the code in the
> future, if the reader has to do non-trivial investigation just to know
> what's going on, it's hurting our work.
>
> If the win is to avoid typing I'd say it's not worth the risk.  Or
> allow it, but only with commentary as to why it's safe, or with rules
> about what sort of types it's allowed with and anything other than those
> requires justification in comments/etc.
>
> --
> Randell Jesup, Mozilla Corp
> remove "news" for personal email
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Hiding 'new' statements - Good or Evil?

[Randell Jesup]

[SNIP]
>> If foo->bar() can mutate the lifetime of foo, of course you must take a 
>> strong
>> reference to foo. Nothing about auto or range-for changes this.
>
>What auto does is make it really hard to tell whether a strong reference is
>being taken.
>
>> If you don't understand your lifetimes, you get bugs.
>
>Fully agreed.  The discussion is about whether auto helps or hinders that
>understanding.  The answer is that it depends on the surrounding code, from
>where I sit...
>
>-Boris

So, where do we go from here?

It's clear that auto is not as safe as some/many believe it to be; it
can as Boris (and smaug and Xidorn) say hide lifetime issues and lead to
non-obvious UAFs and the like.  Some uses are certainly safe, but it
isn't always obvious looking at a patch (per above), requiring more code
investigation by the reviewer.  If the resultant type isn't obvious,
then using it isn't helping comprehension.  When reading the code in the
future, if the reader has to do non-trivial investigation just to know
what's going on, it's hurting our work.

If the win is to avoid typing I'd say it's not worth the risk.  Or
allow it, but only with commentary as to why it's safe, or with rules
about what sort of types it's allowed with and anything other than those
requires justification in comments/etc.

-- 
Randell Jesup, Mozilla Corp
remove "news" for personal email
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Hiding 'new' statements - Good or Evil?

[Boris Zbarsky]

On 11/28/17 10:28 PM, Jeff Gilbert wrote:

const auto& foo = getFoo();
foo->bar(); // foo is always safe here


Sadly not true.  Some counterexamples:

  Foo* getFoo() {
return mFoo; // Hey, this is the common case!
  }

  const RefPtr& getFoo() {
return mFoo;  // Can be nulled out by the call to bar()
  }

It's safe if you have:

  RefPtr getFoo() {
return mFoo;
  }

but of course now every access has refcounting, which is not exactly 0-cost.


Use of auto instead of auto& should be less common.


At a quick glance, we have ~16000 mentions of "auto" in our .h/.cpp 
files (some in comments, I expect).  ~2000 are "auto&", ~800 are "auto 
&", ~700 are "auto*", ~200 are "auto *".


There's a lot of

  for (auto iter = mObserverTopicTable.Iter(); !iter.Done(); iter.Next()) {

going on (~1000).

There's also things like:

  for (auto entry : aLookAndFeelIntCache) {

and whatnot.


Almost every other auto is `const auto&`, which has quite good and
predictable behavior: It names a temporary and extends its lifetime to
the current scope.


That's fine, but the temporary may not keep the thing you're dealing 
with alive; see above.



   for (auto foo : myFoos) {
 foo->bar();
   }
This is always safe if decltype(foo) is a strong ref.


Which it may or may not be; that depends on what the iterator defined on 
myFoos do.



If foo->bar() can mutate the lifetime of foo, of course you must take a strong
reference to foo. Nothing about auto or range-for changes this.


What auto does is make it really hard to tell whether a strong reference 
is being taken.



If you don't understand your lifetimes, you get bugs.


Fully agreed.  The discussion is about whether auto helps or hinders 
that understanding.  The answer is that it depends on the surrounding 
code, from where I sit...


-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Hiding 'new' statements - Good or Evil?

[Jeff Gilbert]

const auto& foo = getFoo();
foo->bar(); // foo is always safe here

Use of auto instead of auto& should be less common. I only use it for:
Casts: `const auto foo = static_cast(bar);`
Or long (but obvious) types I need a value of, usually RefPtrs of long types.

Almost every other auto is `const auto&`, which has quite good and
predictable behavior: It names a temporary and extends its lifetime to
the current scope.


As for ranged-for:
For one, range-based iteration is equivalent to normal iterator
iteration, which is required for traversal of non-linear containers.
For two, modifying a container while doing normal index iteration
still causes buggy behavior, so it can't be a panacea. (particularly
if you hoist the size!)

  for (auto foo : myFoos) {
foo->bar();
  }
This is always safe if decltype(foo) is a strong ref. If foo->bar()
can mutate the lifetime of foo, of course you must take a strong
reference to foo. Nothing about auto or range-for changes this. If
foo->bar() can mutate myFoos, even index iteration is likely to have
bugs.


If you don't understand your lifetimes, you get bugs. Keeping to C++03
doesn't save you. Keeping to c++03 just means you only get the c++03
versions of these bugs. (or in our case, ns/moz-prefixed versions of
these bugs)

If you're reviewing code and can't figure out what the lifetimes are:
R-. I know well that sometimes this is the hardest part of review, but
it's a required and important part of the review, and sometimes the
most important precisely because we can't always use a
sufficiently-restricted set of constructs.

Review is sign-off about "I understand this and it's good (enough)".
If you have particular module-specific issues where
otherwise-acceptable constructs are particularly problematic, sure,
review appropriately. In my modules, these constructs are fine.
Perhaps this is because we have a more-constrained problem space than
say dom/html.

TL;DR:
If you have reason to ban a construct for your module: Cool and normal.
But the bar for banning a construct for all modules must be higher than that.
A mismatch for one module is not sufficient reason to ban it in general.

PS: If any reviewers need a crash course in any of these new concepts,
I'm more than happy to set up office hours at the All-Hands.

On Tue, Nov 28, 2017 at 1:35 PM, smaug  wrote:
> On 11/28/2017 06:33 AM, Boris Zbarsky wrote:
>>
>> On 11/27/17 7:45 PM, Eric Rescorla wrote:
>>>
>>> As for the lifetime question, can you elaborate on the scenario you are
>>> concerned about.
>>
>>
>> Olli may have a different concern, but I'm thinking something like this:
>>
>>   for (auto foo : myFoos) {
>> foo->bar();
>>   }
>>
>
> That was pretty much what I had in mind.
> Though, using auto without range-for, so just
> auto foo = getFoo();
> foo->bar(); // is this safe?
>
>
>
>
>> where bar() can run arbitrary script.  Is "foo" held alive across that
>> call?  Who knows; you have to go read the definition of the iterators on the
>> type of myFoos to find out.
>>
>> One possible answer is that the right solution for this type of issue is
>> the MOZ_CAN_RUN_SCRIPT static analysis annotation on bar(), which will make
>> this code not compile if the type of "foo" is a raw pointer But this
>> annotation is only added to a few functions in our codebase so far, and
>> we'll
>> see how well we manage at adding it to more.  We have a _lot_ of stuff in
>> our codebase that can run random script.  :(
>>
>> -Boris
>
>
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Hiding 'new' statements - Good or Evil?

[smaug]

On 11/28/2017 06:33 AM, Boris Zbarsky wrote:

On 11/27/17 7:45 PM, Eric Rescorla wrote:

As for the lifetime question, can you elaborate on the scenario you are
concerned about.


Olli may have a different concern, but I'm thinking something like this:

  for (auto foo : myFoos) {
foo->bar();
  }



That was pretty much what I had in mind.
Though, using auto without range-for, so just
auto foo = getFoo();
foo->bar(); // is this safe?




where bar() can run arbitrary script.  Is "foo" held alive across that call?  
Who knows; you have to go read the definition of the iterators on the
type of myFoos to find out.

One possible answer is that the right solution for this type of issue is the 
MOZ_CAN_RUN_SCRIPT static analysis annotation on bar(), which will make
this code not compile if the type of "foo" is a raw pointer But this 
annotation is only added to a few functions in our codebase so far, and we'll
see how well we manage at adding it to more.  We have a _lot_ of stuff in our 
codebase that can run random script.  :(

-Boris


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Hiding 'new' statements - Good or Evil?

[Xidorn Quan]

On Wed, Nov 29, 2017, at 12:56 AM, Eric Rescorla wrote:
> On Mon, Nov 27, 2017 at 6:41 PM, Xidorn Quan  wrote:
>> On Tue, Nov 28, 2017, at 11:45 AM, Eric Rescorla wrote:
>>  > On Mon, Nov 27, 2017 at 4:07 PM, smaug  wrote:
>>  > > And auto makes code reading harder. It hides important
>>  > > information like lifetime management. It happens easily with
>>  > > auto that one doesn't even start to think whether
>>  > > nsCOMPtr/RefPtr should be used there.
>>  >
>>  > This statement seems perhaps a touch to universalist; it may be
>>  > the case that it makes it harder for *you* to read, but I find it
>>  > makes it easier for me to read. I also don't think it's a
>>  > coincidence that it's a common feature of modern languages (Rust
>>  > and Go, to name just two), so I suspect I'm not alone in thinking
>>  > auto is a good thing.
>>
>> Using Rust and Go as examples isn't very fair.>> 
>>  Go has GC, and Rust has compile-time lifetime checking, so they are>>  
>> basically free from certain kind of lifetime issues.
> 
> I don't think that it's unfair. My point was that these are features
> that language designers and users think are desirable, which is why
> they get added to languages. After all, you could certainly build a
> language like Rust or Go that didn't have any type inference; people
> would just wonder what you were thinking. And I strongly suspect they
> got added to C++ for the same reason.> 
> With that said, this kind of lifetime issue in C++ largely arises from
> the use of raw pointers (or auto_ptr, I guess, though that's been
> deprecated), and in the specific cases under discussion here, by
> intermixing old-style memory management with a bunch of the newer
> features. We could of course deal with that by not letting anybody use
> the new features, but it seems like a more promising direction would
> be to move to a more modern memory management idiom, which C++-14
> already supports.
Yes and no. That kind of lifetime issues are mainly raised from the use
of raw pointers (as well as references, actually), but how do you (in
your imagination) plan to get rid of all the raw pointers in the wild?
Replace every code which currently returns / holds a raw pointer with a
RefPtr / UniquePtr instead? Add weak reference wrapper for every type we
need to weak reference?
I don't believe that's something realistic.

Even ignoring the scale of code needs to change, this kind of "modern
memory management" would likely come with a runtime cost in C++ (rather
than mostly compile-time cost in Rust). It is more similar to the
runtime cost Go would pay for its GC, but even worse, a refcount-based
memory management system would generally be more expensive on runtime
than a tracing GC. And I don't think that's a cost we (or most other C++
users) want to pay.
To be honest, I don't really see how (more) modern C++ (14 and up)
really makes memory management any better. There are more new features
which tend to hide lifetime issues and let users bite the bullet
themselves. Taking my example above again, std::string_view, which is
added in C++17, can easily lead to UAF without any raw pointer usage in
user code. How would the modern memory management idiom (which according
to you, C++14 have already supported) help here?
I'm not saying that we shouldn't let anybody use the new features. But
certain new features really need more carefulness. They can be used when
they wouldn't footgun, but they should be avoided otherwise. A new
features' being there doesn't mean we must use it everywhere, and it
will really make things better.
- Xidorn
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Hiding 'new' statements - Good or Evil?

[Eric Rescorla]

On Mon, Nov 27, 2017 at 6:41 PM, Xidorn Quan  wrote:

> On Tue, Nov 28, 2017, at 11:45 AM, Eric Rescorla wrote:
> > On Mon, Nov 27, 2017 at 4:07 PM, smaug  wrote:
> > > And auto makes code reading harder. It hides important information like
> > > lifetime management.
> > > It happens easily with auto that one doesn't even start to think
> whether
> > > nsCOMPtr/RefPtr should be used there.
> >
> > This statement seems perhaps a touch to universalist; it may be the case
> > that it makes it harder for *you* to read, but I find it makes it easier
> > for me to read. I also don't think it's a coincidence that it's a common
> > feature of modern languages (Rust and Go, to name just two), so I suspect
> > I'm not alone in thinking auto is a good thing.
>
> Using Rust and Go as examples isn't very fair.
>
> Go has GC, and Rust has compile-time lifetime checking, so they are
> basically free from certain kind of lifetime issues.


I don't think that it's unfair. My point was that these are features that
language designers and users think are desirable, which is why they get
added to languages. After all, you could certainly build a language like
Rust or Go that didn't have any type inference; people would just wonder
what you were thinking. And I strongly suspect they got added to C++ for
the same reason.

With that said, this kind of lifetime issue in C++ largely arises from the
use of raw pointers (or auto_ptr, I guess, though that's been deprecated),
and in the specific cases under discussion here, by intermixing old-style
memory management with a bunch of the newer features. We could of course
deal with that by not letting anybody use the new features, but it seems
like a more promising direction would be to move to a more modern memory
management idiom, which C++-14 already supports.

-Ekr
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Hiding 'new' statements - Good or Evil?

[Boris Zbarsky]

On 11/27/17 7:45 PM, Eric Rescorla wrote:

As for the lifetime question, can you elaborate on the scenario you are
concerned about.


Olli may have a different concern, but I'm thinking something like this:

  for (auto foo : myFoos) {
foo->bar();
  }

where bar() can run arbitrary script.  Is "foo" held alive across that 
call?  Who knows; you have to go read the definition of the iterators on 
the type of myFoos to find out.


One possible answer is that the right solution for this type of issue is 
the MOZ_CAN_RUN_SCRIPT static analysis annotation on bar(), which will 
make this code not compile if the type of "foo" is a raw pointer 
But this annotation is only added to a few functions in our codebase so 
far, and we'll see how well we manage at adding it to more.  We have a 
_lot_ of stuff in our codebase that can run random script.  :(


-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Hiding 'new' statements - Good or Evil?

[Xidorn Quan]

On Tue, Nov 28, 2017, at 11:45 AM, Eric Rescorla wrote:
> On Mon, Nov 27, 2017 at 4:07 PM, smaug  wrote:
> > And auto makes code reading harder. It hides important information like
> > lifetime management.
> > It happens easily with auto that one doesn't even start to think whether
> > nsCOMPtr/RefPtr should be used there.
> 
> This statement seems perhaps a touch to universalist; it may be the case
> that it makes it harder for *you* to read, but I find it makes it easier
> for me to read. I also don't think it's a coincidence that it's a common
> feature of modern languages (Rust and Go, to name just two), so I suspect
> I'm not alone in thinking auto is a good thing.

Using Rust and Go as examples isn't very fair.

Go has GC, and Rust has compile-time lifetime checking, so they are
basically free from certain kind of lifetime issues. There are features
in Rust that can easily become footgun when used with C++, one example
is std::string_view. [1]

[1] https://github.com/isocpp/CppCoreGuidelines/issues/1038

- Xidorn
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Hiding 'new' statements - Good or Evil?

[Eric Rescorla]

On Mon, Nov 27, 2017 at 4:07 PM, smaug  wrote:

> On 11/28/2017 12:53 AM, Jeff Gilbert wrote:
>
>> ranged-for issues are the same as those for doing manual iteration,
>>
> It is not, in case you iterate using
> for (i = 0; i < foo.length(); ++i)
> And that is the case which has been often converted to ranged-for, with
> bad results.
>
> And auto makes code reading harder. It hides important information like
> lifetime management.
> It happens easily with auto that one doesn't even start to think whether
> nsCOMPtr/RefPtr should be used there.
>

This statement seems perhaps a touch to universalist; it may be the case
that it makes it harder for *you* to read, but I find it makes it easier
for me to read. I also don't think it's a coincidence that it's a common
feature of modern languages (Rust and Go, to name just two), so I suspect
I'm not alone in thinking auto is a good thing.

As for the lifetime question, can you elaborate on the scenario you are
concerned about. Is it something like:

T *foo() {
   return new T();
}

void bar() {
   auto t = foo();
}

Where you think that t should be assigned to a smart pointer? (Obviously,
there are ways for this to cause UAF as well, though this one is a leak).

-Ekr
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Hiding 'new' statements - Good or Evil?

[Michael Froman]

On Nov 27, 2017, at 6:07 PM, smaug  wrote:
> 
> And auto makes code reading harder. It hides important information like 
> lifetime management.
> It happens easily with auto that one doesn't even start to think whether 
> nsCOMPtr/RefPtr should be used there.
> 
> I'm saying this all with my reviewer's hat on, after reading code which got 
> committed to m-c with proper r+
> (even from experienced engineers).
> 
+1 (especially on the review side of things)

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Hiding 'new' statements - Good or Evil?

[smaug]

On 11/28/2017 12:53 AM, Jeff Gilbert wrote:

ranged-for issues are the same as those for doing manual iteration,

It is not, in case you iterate using
for (i = 0; i < foo.length(); ++i)
And that is the case which has been often converted to ranged-for, with bad 
results.

And auto makes code reading harder. It hides important information like 
lifetime management.
It happens easily with auto that one doesn't even start to think whether 
nsCOMPtr/RefPtr should be used there.

I'm saying this all with my reviewer's hat on, after reading code which got 
committed to m-c with proper r+
(even from experienced engineers).





and your complaints about auto are caused by deficient code/design
review.

The further we deviate from standards, the harder it is for
contributors (and not just new ones) to do the right thing. The
default should be to move towards the standard, not cringe back from
it. Sure, prove it out. But we really don't need more moz-specific
constructs. We should choose our deviations carefully.

On Mon, Nov 27, 2017 at 3:24 AM, smaug  wrote:

On 11/27/2017 01:05 PM, Nicolas B. Pierron wrote:


On 11/26/2017 12:45 AM, smaug wrote:


On 11/24/2017 06:35 PM, Eric Rescorla wrote:


On Thu, Nov 23, 2017 at 4:00 PM, smaug  wrote:


On 11/23/2017 11:54 PM, Botond Ballo wrote:


I think it makes sense to hide a 'new' call in a Make* function when
you're writing an abstraction that handles allocation *and*
deallocation.

So MakeUnique makes sense, because UniquePtr takes ownership of the
allocated object, and will deallocate it in the destructor. MakeRefPtr
would make sense for the same reason.


I almost agree with this, but, all these Make* variants hide the
information that they are allocating,
and since allocation is slow, it is usually better to know when
allocation
happens.
I guess I'd prefer UniquePtr::New() over MakeUnique to be more clear
about
the functionality.



This seems like a reasonable argument in isolation, but I think it's
more
important to mirror the standard C++ mechanisms and C++-14 already
defines
std::make_unique.



Is it? Isn't it more important to write less error prone code and code
where the reader
sees the possible performance issues.
Recent-ish C++ additions have brought quite a few bad concepts which we
shouldn't use
without considering whether they actually fit into our needs.
Something being new and hip and cool doesn't mean it is actually a good
thing.



One example, is that the standard has a single std::function type which
always allocate, and does not report failures because we disable exceptions.

In this particular case I would recommend to have a stack-function wrapper
and a heap-function wrapper. By diverging from the standard we could avoid
bugs such as refusing stack-references for lambda being wrapped in
heap-function (use-after-free/unwind), and adding extra checks that
stack-function
wrapper can only be fields of stack-only classes.  C++ compilers have no
way to understand the code enough to provide similar life-time errors.


(This broken record keeps reminding about the security issues and memory
leaks auto and ranged-for have caused.)



Do you have pointers? Is this basically when we have a non-standard
implementation of begin/end methods which are doing allocation?



ranged-for causes issues when you modify the data structure while iterating.
This used to be bad in nsTArray case at least, now we just crash safely.


And auto hides the type, so reading the code and understanding lifetime
management becomes harder.
This has caused security sensitive crashes and leaks. (And there was some
other type of error too recently, but it escapes my mind now)








As a post-script, given that we now can use C++-14, can we globally
replace
the MFBT clones of C++-14 mechanisms with the standard ones?

-Ekr





-Olli




But in cases where a library facility is not taking ownership of the
object, and thus the user will need to write an explicit 'delete', it
makes sense to require that the user also write an explicit 'new', for
symmetry.

NotNull is a bit of a weird case because it can wrap either a raw
pointer or a smart pointer, so it doesn't clearly fit into either
category. Perhaps it would make sense for MakeNotNull to only be
usable with smart pointers?

Cheers,
Botond



___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform








___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Hiding 'new' statements - Good or Evil?

[Jeff Gilbert]

ranged-for issues are the same as those for doing manual iteration,
and your complaints about auto are caused by deficient code/design
review.

The further we deviate from standards, the harder it is for
contributors (and not just new ones) to do the right thing. The
default should be to move towards the standard, not cringe back from
it. Sure, prove it out. But we really don't need more moz-specific
constructs. We should choose our deviations carefully.

On Mon, Nov 27, 2017 at 3:24 AM, smaug  wrote:
> On 11/27/2017 01:05 PM, Nicolas B. Pierron wrote:
>>
>> On 11/26/2017 12:45 AM, smaug wrote:
>>>
>>> On 11/24/2017 06:35 PM, Eric Rescorla wrote:

 On Thu, Nov 23, 2017 at 4:00 PM, smaug  wrote:

> On 11/23/2017 11:54 PM, Botond Ballo wrote:
>
>> I think it makes sense to hide a 'new' call in a Make* function when
>> you're writing an abstraction that handles allocation *and*
>> deallocation.
>>
>> So MakeUnique makes sense, because UniquePtr takes ownership of the
>> allocated object, and will deallocate it in the destructor. MakeRefPtr
>> would make sense for the same reason.
>>
> I almost agree with this, but, all these Make* variants hide the
> information that they are allocating,
> and since allocation is slow, it is usually better to know when
> allocation
> happens.
> I guess I'd prefer UniquePtr::New() over MakeUnique to be more clear
> about
> the functionality.
>

 This seems like a reasonable argument in isolation, but I think it's
 more
 important to mirror the standard C++ mechanisms and C++-14 already
 defines
 std::make_unique.
>>>
>>>
>>> Is it? Isn't it more important to write less error prone code and code
>>> where the reader
>>> sees the possible performance issues.
>>> Recent-ish C++ additions have brought quite a few bad concepts which we
>>> shouldn't use
>>> without considering whether they actually fit into our needs.
>>> Something being new and hip and cool doesn't mean it is actually a good
>>> thing.
>>
>>
>> One example, is that the standard has a single std::function type which
>> always allocate, and does not report failures because we disable exceptions.
>>
>> In this particular case I would recommend to have a stack-function wrapper
>> and a heap-function wrapper. By diverging from the standard we could avoid
>> bugs such as refusing stack-references for lambda being wrapped in
>> heap-function (use-after-free/unwind), and adding extra checks that
>> stack-function
>> wrapper can only be fields of stack-only classes.  C++ compilers have no
>> way to understand the code enough to provide similar life-time errors.
>>
>>> (This broken record keeps reminding about the security issues and memory
>>> leaks auto and ranged-for have caused.)
>>
>>
>> Do you have pointers? Is this basically when we have a non-standard
>> implementation of begin/end methods which are doing allocation?
>
>
> ranged-for causes issues when you modify the data structure while iterating.
> This used to be bad in nsTArray case at least, now we just crash safely.
>
>
> And auto hides the type, so reading the code and understanding lifetime
> management becomes harder.
> This has caused security sensitive crashes and leaks. (And there was some
> other type of error too recently, but it escapes my mind now)
>
>
>>
>>>

 As a post-script, given that we now can use C++-14, can we globally
 replace
 the MFBT clones of C++-14 mechanisms with the standard ones?

 -Ekr


>
>
> -Olli
>
>
>
>> But in cases where a library facility is not taking ownership of the
>> object, and thus the user will need to write an explicit 'delete', it
>> makes sense to require that the user also write an explicit 'new', for
>> symmetry.
>>
>> NotNull is a bit of a weird case because it can wrap either a raw
>> pointer or a smart pointer, so it doesn't clearly fit into either
>> category. Perhaps it would make sense for MakeNotNull to only be
>> usable with smart pointers?
>>
>> Cheers,
>> Botond
>>
>>
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
>>>
>>
>>
>
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Hiding 'new' statements - Good or Evil?

[smaug]

On 11/27/2017 01:05 PM, Nicolas B. Pierron wrote:

On 11/26/2017 12:45 AM, smaug wrote:

On 11/24/2017 06:35 PM, Eric Rescorla wrote:

On Thu, Nov 23, 2017 at 4:00 PM, smaug  wrote:


On 11/23/2017 11:54 PM, Botond Ballo wrote:


I think it makes sense to hide a 'new' call in a Make* function when
you're writing an abstraction that handles allocation *and*
deallocation.

So MakeUnique makes sense, because UniquePtr takes ownership of the
allocated object, and will deallocate it in the destructor. MakeRefPtr
would make sense for the same reason.


I almost agree with this, but, all these Make* variants hide the
information that they are allocating,
and since allocation is slow, it is usually better to know when allocation
happens.
I guess I'd prefer UniquePtr::New() over MakeUnique to be more clear about
the functionality.



This seems like a reasonable argument in isolation, but I think it's more
important to mirror the standard C++ mechanisms and C++-14 already defines
std::make_unique.


Is it? Isn't it more important to write less error prone code and code where 
the reader
sees the possible performance issues.
Recent-ish C++ additions have brought quite a few bad concepts which we 
shouldn't use
without considering whether they actually fit into our needs.
Something being new and hip and cool doesn't mean it is actually a good thing.


One example, is that the standard has a single std::function type which always 
allocate, and does not report failures because we disable exceptions.

In this particular case I would recommend to have a stack-function wrapper and 
a heap-function wrapper. By diverging from the standard we could avoid
bugs such as refusing stack-references for lambda being wrapped in 
heap-function (use-after-free/unwind), and adding extra checks that 
stack-function
wrapper can only be fields of stack-only classes.  C++ compilers have no way to 
understand the code enough to provide similar life-time errors.


(This broken record keeps reminding about the security issues and memory leaks 
auto and ranged-for have caused.)


Do you have pointers? Is this basically when we have a non-standard 
implementation of begin/end methods which are doing allocation?


ranged-for causes issues when you modify the data structure while iterating.
This used to be bad in nsTArray case at least, now we just crash safely.


And auto hides the type, so reading the code and understanding lifetime 
management becomes harder.
This has caused security sensitive crashes and leaks. (And there was some other 
type of error too recently, but it escapes my mind now)







As a post-script, given that we now can use C++-14, can we globally replace
the MFBT clones of C++-14 mechanisms with the standard ones?

-Ekr





-Olli




But in cases where a library facility is not taking ownership of the
object, and thus the user will need to write an explicit 'delete', it
makes sense to require that the user also write an explicit 'new', for
symmetry.

NotNull is a bit of a weird case because it can wrap either a raw
pointer or a smart pointer, so it doesn't clearly fit into either
category. Perhaps it would make sense for MakeNotNull to only be
usable with smart pointers?

Cheers,
Botond



___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform








___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Hiding 'new' statements - Good or Evil?

[Nicolas B. Pierron]

On 11/26/2017 12:45 AM, smaug wrote:

On 11/24/2017 06:35 PM, Eric Rescorla wrote:

On Thu, Nov 23, 2017 at 4:00 PM, smaug  wrote:


On 11/23/2017 11:54 PM, Botond Ballo wrote:


I think it makes sense to hide a 'new' call in a Make* function when
you're writing an abstraction that handles allocation *and*
deallocation.

So MakeUnique makes sense, because UniquePtr takes ownership of the
allocated object, and will deallocate it in the destructor. MakeRefPtr
would make sense for the same reason.


I almost agree with this, but, all these Make* variants hide the
information that they are allocating,
and since allocation is slow, it is usually better to know when allocation
happens.
I guess I'd prefer UniquePtr::New() over MakeUnique to be more clear about
the functionality.



This seems like a reasonable argument in isolation, but I think it's more
important to mirror the standard C++ mechanisms and C++-14 already defines
std::make_unique.


Is it? Isn't it more important to write less error prone code and code where 
the reader

sees the possible performance issues.
Recent-ish C++ additions have brought quite a few bad concepts which we 
shouldn't use

without considering whether they actually fit into our needs.
Something being new and hip and cool doesn't mean it is actually a good thing.


One example, is that the standard has a single std::function type which 
always allocate, and does not report failures because we disable exceptions.


In this particular case I would recommend to have a stack-function wrapper 
and a heap-function wrapper. By diverging from the standard we could avoid 
bugs such as refusing stack-references for lambda being wrapped in 
heap-function (use-after-free/unwind), and adding extra checks that 
stack-function wrapper can only be fields of stack-only classes.  C++ 
compilers have no way to understand the code enough to provide similar 
life-time errors.


(This broken record keeps reminding about the security issues and memory 
leaks auto and ranged-for have caused.)


Do you have pointers? Is this basically when we have a non-standard 
implementation of begin/end methods which are doing allocation?






As a post-script, given that we now can use C++-14, can we globally replace
the MFBT clones of C++-14 mechanisms with the standard ones?

-Ekr





-Olli




But in cases where a library facility is not taking ownership of the
object, and thus the user will need to write an explicit 'delete', it
makes sense to require that the user also write an explicit 'new', for
symmetry.

NotNull is a bit of a weird case because it can wrap either a raw
pointer or a smart pointer, so it doesn't clearly fit into either
category. Perhaps it would make sense for MakeNotNull to only be
usable with smart pointers?

Cheers,
Botond



___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform






--
Nicolas B. Pierron
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Hiding 'new' statements - Good or Evil?

[Nathan Froyd]

On Fri, Nov 24, 2017 at 11:35 AM, Eric Rescorla  wrote:
> On Thu, Nov 23, 2017 at 4:00 PM, smaug  wrote:
>> I guess I'd prefer UniquePtr::New() over MakeUnique to be more clear about
>> the functionality.
>
> This seems like a reasonable argument in isolation, but I think it's more
> important to mirror the standard C++ mechanisms and C++-14 already defines
> std::make_unique.
>
> As a post-script, given that we now can use C++-14, can we globally replace
> the MFBT clones of C++-14 mechanisms with the standard ones?

In general, no.

std::unique_ptr is not a drop-in replacement for mozilla::UniquePtr,
for instance, and people seem OK with that--plus maintaining our own
smart pointer class opens up optimization opportunities the standard
library can only implement with difficulty[0].  In a similar fashion,
having our own mozilla::Move means we can implement checks on the use
of Move that the standard library version can't.  std::vector is not
an adequate replacement for mozilla::Vector.  And so forth.

There are specific instances where using the standard library doesn't
seem problematic: we have a bug open on replacing TypeTraits.h with
type_traits, but that has run into issues with how we wrap STL
headers, and nobody has devoted the time to figuring out how to deal
with the issues.  But each instance would have to be considered on the
merits.

-Nathan

[0] See http://lists.llvm.org/pipermail/cfe-dev/2017-November/055955.html
and the ensuing discussion.  There was discussion of standardizing
this attribute, but it's not clear to me that std::unique_ptr could
immediately take advantage of this without ABI breakage.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Hiding 'new' statements - Good or Evil?

[smaug]

On 11/24/2017 06:35 PM, Eric Rescorla wrote:

On Thu, Nov 23, 2017 at 4:00 PM, smaug  wrote:


On 11/23/2017 11:54 PM, Botond Ballo wrote:


I think it makes sense to hide a 'new' call in a Make* function when
you're writing an abstraction that handles allocation *and*
deallocation.

So MakeUnique makes sense, because UniquePtr takes ownership of the
allocated object, and will deallocate it in the destructor. MakeRefPtr
would make sense for the same reason.


I almost agree with this, but, all these Make* variants hide the
information that they are allocating,
and since allocation is slow, it is usually better to know when allocation
happens.
I guess I'd prefer UniquePtr::New() over MakeUnique to be more clear about
the functionality.



This seems like a reasonable argument in isolation, but I think it's more
important to mirror the standard C++ mechanisms and C++-14 already defines
std::make_unique.


Is it? Isn't it more important to write less error prone code and code where 
the reader
sees the possible performance issues.
Recent-ish C++ additions have brought quite a few bad concepts which we 
shouldn't use
without considering whether they actually fit into our needs.
Something being new and hip and cool doesn't mean it is actually a good thing.

(This broken record keeps reminding about the security issues and memory leaks 
auto and ranged-for have caused.)




As a post-script, given that we now can use C++-14, can we globally replace
the MFBT clones of C++-14 mechanisms with the standard ones?

-Ekr





-Olli




But in cases where a library facility is not taking ownership of the
object, and thus the user will need to write an explicit 'delete', it
makes sense to require that the user also write an explicit 'new', for
symmetry.

NotNull is a bit of a weird case because it can wrap either a raw
pointer or a smart pointer, so it doesn't clearly fit into either
category. Perhaps it would make sense for MakeNotNull to only be
usable with smart pointers?

Cheers,
Botond



___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform



___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Hiding 'new' statements - Good or Evil?

[Eric Rescorla]

On Thu, Nov 23, 2017 at 4:00 PM, smaug  wrote:

> On 11/23/2017 11:54 PM, Botond Ballo wrote:
>
>> I think it makes sense to hide a 'new' call in a Make* function when
>> you're writing an abstraction that handles allocation *and*
>> deallocation.
>>
>> So MakeUnique makes sense, because UniquePtr takes ownership of the
>> allocated object, and will deallocate it in the destructor. MakeRefPtr
>> would make sense for the same reason.
>>
> I almost agree with this, but, all these Make* variants hide the
> information that they are allocating,
> and since allocation is slow, it is usually better to know when allocation
> happens.
> I guess I'd prefer UniquePtr::New() over MakeUnique to be more clear about
> the functionality.
>

This seems like a reasonable argument in isolation, but I think it's more
important to mirror the standard C++ mechanisms and C++-14 already defines
std::make_unique.

As a post-script, given that we now can use C++-14, can we globally replace
the MFBT clones of C++-14 mechanisms with the standard ones?

-Ekr


>
>
> -Olli
>
>
>
>> But in cases where a library facility is not taking ownership of the
>> object, and thus the user will need to write an explicit 'delete', it
>> makes sense to require that the user also write an explicit 'new', for
>> symmetry.
>>
>> NotNull is a bit of a weird case because it can wrap either a raw
>> pointer or a smart pointer, so it doesn't clearly fit into either
>> category. Perhaps it would make sense for MakeNotNull to only be
>> usable with smart pointers?
>>
>> Cheers,
>> Botond
>>
>>
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Hiding 'new' statements - Good or Evil?

[smaug]

On 11/23/2017 11:54 PM, Botond Ballo wrote:

I think it makes sense to hide a 'new' call in a Make* function when
you're writing an abstraction that handles allocation *and*
deallocation.

So MakeUnique makes sense, because UniquePtr takes ownership of the
allocated object, and will deallocate it in the destructor. MakeRefPtr
would make sense for the same reason.

I almost agree with this, but, all these Make* variants hide the information 
that they are allocating,
and since allocation is slow, it is usually better to know when allocation 
happens.
I guess I'd prefer UniquePtr::New() over MakeUnique to be more clear about the 
functionality.



-Olli




But in cases where a library facility is not taking ownership of the
object, and thus the user will need to write an explicit 'delete', it
makes sense to require that the user also write an explicit 'new', for
symmetry.

NotNull is a bit of a weird case because it can wrap either a raw
pointer or a smart pointer, so it doesn't clearly fit into either
category. Perhaps it would make sense for MakeNotNull to only be
usable with smart pointers?

Cheers,
Botond



___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Hiding 'new' statements - Good or Evil?

[Botond Ballo]

I think it makes sense to hide a 'new' call in a Make* function when
you're writing an abstraction that handles allocation *and*
deallocation.

So MakeUnique makes sense, because UniquePtr takes ownership of the
allocated object, and will deallocate it in the destructor. MakeRefPtr
would make sense for the same reason.

But in cases where a library facility is not taking ownership of the
object, and thus the user will need to write an explicit 'delete', it
makes sense to require that the user also write an explicit 'new', for
symmetry.

NotNull is a bit of a weird case because it can wrap either a raw
pointer or a smart pointer, so it doesn't clearly fit into either
category. Perhaps it would make sense for MakeNotNull to only be
usable with smart pointers?

Cheers,
Botond
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Hiding 'new' statements - Good or Evil?

[Jonathan Kew]

On 23/11/2017 12:05, Jeff Gilbert wrote:

I agree that MakeNotNull doesn't sound like it allocates. Reads to me
as Make[Into]NotNull. Context will *usually* make this clear, but why
not NewNotNull? (Honestly I don't like MakeUnique to begin with)


As far as naming is concerned, ISTM that "Create..." would be clearer 
than "Make..." for methods like this, as it's not so easy to 
misinterpret as "make [an existing thing] into [a certain state]".


JK
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Hiding 'new' statements - Good or Evil?

[Jeff Gilbert]

I agree that MakeNotNull doesn't sound like it allocates. Reads to me
as Make[Into]NotNull. Context will *usually* make this clear, but why
not NewNotNull? (Honestly I don't like MakeUnique to begin with)

NotNull::New(args...) is another option.

"My first goal was to avoid the unnecessary nullptr-check in the
NotNull constructor, since our new is infallible by default."
Even if we can't communicate to the optimizer that new is infallible
and incur it to omit the branch in this inlined code, it would be
extremely well predicted.
MOZ_UNLIKELY() it and call it good, IMO.

On Thu, Nov 23, 2017 at 12:58 AM,   wrote:
> I'm not sure what the benefits are (the MakeUnique comments only really seem 
> to give aesthetic ones), but they do make it a bit harder when searching 
> through code to see where things are constructed.
>
> I suppose if we always stick to using Make* then (with regex) it wouldn't add 
> too much burden for searching.
>
> On Thursday, 23 November 2017 07:50:27 UTC, gsqu...@mozilla.com  wrote:
>> Should we allow hiding 'new' statements, or keep them as visible as possible?
>>
>>
>> Some context:
>> Recently in bug 1410252 I added a MakeNotNull(args...) function that 
>> does `NotNull(new T(args...))`, in the style of MakeUnique and others. 
>> It also works with RefPtr.
>>
>> My first goal was to avoid the unnecessary nullptr-check in the NotNull 
>> constructor, since our new is infallible by default.
>>
>> And I thought that hiding the naked new statement was a nice side benefit, 
>> as I was under the impression that it was A Good Thing in modern C++. 
>> (Though I think the main reason for that, was to prevent leaks when handling 
>> exceptions in multi-allocation expressions, so it doesn't apply to us here.)
>>
>> Now, a colleague remarked that this was making the memory allocation less 
>> obvious.
>> "What about MakeUnique?" I asked; "I'm used to them now" he retorted. :-P
>>
>>
>> So, what do you all think?
>> - Should I remove MakeNotNull?
>> - Or should we allow/encourage more MakeX functions instead of X(new...)? 
>> I'm thinking MakeRefPtr might be nice.
>
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Hiding 'new' statements - Good or Evil?

[bowen]

I'm not sure what the benefits are (the MakeUnique comments only really seem to 
give aesthetic ones), but they do make it a bit harder when searching through 
code to see where things are constructed.

I suppose if we always stick to using Make* then (with regex) it wouldn't add 
too much burden for searching.

On Thursday, 23 November 2017 07:50:27 UTC, gsqu...@mozilla.com  wrote:
> Should we allow hiding 'new' statements, or keep them as visible as possible?
> 
> 
> Some context:
> Recently in bug 1410252 I added a MakeNotNull(args...) function that does 
> `NotNull(new T(args...))`, in the style of MakeUnique and others. It also 
> works with RefPtr.
> 
> My first goal was to avoid the unnecessary nullptr-check in the NotNull 
> constructor, since our new is infallible by default.
> 
> And I thought that hiding the naked new statement was a nice side benefit, as 
> I was under the impression that it was A Good Thing in modern C++. (Though I 
> think the main reason for that, was to prevent leaks when handling exceptions 
> in multi-allocation expressions, so it doesn't apply to us here.)
> 
> Now, a colleague remarked that this was making the memory allocation less 
> obvious.
> "What about MakeUnique?" I asked; "I'm used to them now" he retorted. :-P
> 
> 
> So, what do you all think?
> - Should I remove MakeNotNull?
> - Or should we allow/encourage more MakeX functions instead of X(new...)? I'm 
> thinking MakeRefPtr might be nice.

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Hiding 'new' statements - Good or Evil?

[gsquelart]

Should we allow hiding 'new' statements, or keep them as visible as possible?


Some context:
Recently in bug 1410252 I added a MakeNotNull(args...) function that does 
`NotNull(new T(args...))`, in the style of MakeUnique and others. It also 
works with RefPtr.

My first goal was to avoid the unnecessary nullptr-check in the NotNull 
constructor, since our new is infallible by default.

And I thought that hiding the naked new statement was a nice side benefit, as I 
was under the impression that it was A Good Thing in modern C++. (Though I 
think the main reason for that, was to prevent leaks when handling exceptions 
in multi-allocation expressions, so it doesn't apply to us here.)

Now, a colleague remarked that this was making the memory allocation less 
obvious.
"What about MakeUnique?" I asked; "I'm used to them now" he retorted. :-P


So, what do you all think?
- Should I remove MakeNotNull?
- Or should we allow/encourage more MakeX functions instead of X(new...)? I'm 
thinking MakeRefPtr might be nice.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform