Prohibit passing MOZ_RAII classes by-value to functions

NEW
Unassigned

Status

()

Core
Rewriting and Analysis
2 years ago
11 months ago

People

(Reporter: mystor, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Generally accepting a RAII guard as a function argument by value is wrong. You usually want to pass the guard by reference (as passing it by value requires copying it - which is usually wrong).

This could be done by adding a new attribute (MOZ_NO_BYVALUE_ARG_CLASS?) which prevents the type from being passed by-value as an argument, and then adding that attribute to MOZ_RAII.

I'm not sure how much code this would break, or if there are valid use-cases for passing RAII guards by value to functions. So this is also part of this bug.
(Reporter)

Comment 1

2 years ago
Created attachment 8662469 [details] [diff] [review]
Part 1: Add an analysis to detect passing certain classes by value to functions
Attachment #8662469 - Flags: review?(ehsan)
(Reporter)

Comment 2

2 years ago
Created attachment 8662470 [details] [diff] [review]
Part 2: Add MOZ_NO_BY_VALUE_ARG to MFBT, and include it in the definition of MOZ_RAII
Attachment #8662470 - Flags: review?(nfroyd)
(Reporter)

Updated

2 years ago
Depends on: 1205733
Attachment #8662470 - Flags: review?(nfroyd) → review+

Comment 3

2 years ago
I understand why you wouldn't want to pass an RAII thing by /copy/, but how do you pass one by /move/? (Both of these fall under "by value", right?)

Will bug 1153277 take care of this by ensuring there's no copy constructor?
I might be misunderstanding your question, but wouldn't you use a universal (&&) reference to "pass by move"?
(Reporter)

Comment 5

2 years ago
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #4)
> I might be misunderstanding your question, but wouldn't you use a universal
> (&&) reference to "pass by move"?

Technically an && reference is called an r-value reference, unless the type parameter T is deduced, in which case it is a universal reference, as the type perameyer can be deduced to T&& or T&. (yes c++ is terrible).

Anyways, the r-value reference doesn't necessarily move the value, its just a way of indicating that you may move the value. In contrast if you construct the value to pass to the function using a move constructor and use by-value calling. Then you can know at the call site that the object is actually moved. 

In practice we don't move raii guards very often. And most raii guards don't have an explicit move constructor, which means that moving those values is just as incorrect as copying them. 

That being said, there are valid cases where you would want move constructors on raii types. In those cases you would also want to allow temporary objects as long as they are used to pass to a constructor or function accepting a rvalue reference, as these types are presumably movable. 

For now, none of the types marked as MOZ_RAII are passed by value, so this is OK as an analysis I think, bug I would agree that a better analysis may be necessary. Namely, we might want to treat a type differently if it is an explicit move constructor.

Comment 6

2 years ago
So this hasn't caught any code, it seems.  What's the motivation behind adding this?
Flags: needinfo?(michael)
(Reporter)

Comment 7

2 years ago
I was talking with fitzgen on IRC, and he was wondering if we had any plans to prevent mistakes around potentially performing incorrect copies of RAII types - this was simply one mechanism for preventing these copies which we came up with, and which would be unlikely to break a lot of code.

I agree that it may be unnecessary.
Flags: needinfo?(michael)

Comment 8

2 years ago
No, it's fine.  I was just trying to understand the goals.
(Reporter)

Comment 9

2 years ago
Comment on attachment 8662469 [details] [diff] [review]
Part 1: Add an analysis to detect passing certain classes by value to functions

Clearing review because patch would be of questionable value.
Attachment #8662469 - Flags: review?(ehsan)
(Reporter)

Updated

11 months ago
Assignee: michael → nobody
You need to log in before you can comment on or make changes to this bug.