Closed
Bug 1377351
Opened 7 years ago
Closed 7 years ago
Consider adding a move constructor and move assign overload/method to nsA[C]String
Categories
(Core :: XPCOM, enhancement)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: nika, Assigned: nika)
References
Details
Attachments
(5 files, 5 obsolete files)
3.02 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
4.71 KB,
patch
|
Details | Diff | Splinter Review | |
8.55 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
7.69 KB,
patch
|
froydnj
:
feedback+
|
Details | Diff | Splinter Review |
8.67 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
I have opinions as to what this should act like. I think that we should attempt to move the buffer in the case that the assigned-to string will not have dependent strings, or strings which don't match the expected semantics of the receiving string type. Namely I imagine that the following should happen: 1. If F_TERMINATED is not set on the source string, perform a copy, and truncate the source string. 2. If F_SHARED, F_OWNED, or F_LITERAL is set on the source string, transfer ownership (or the reference to in the case of F_LITERAL) of the backing buffer to the target string, and truncate the source string. 3. For all other strings, perform a copy, and truncate the source string. This ensures that the target string always has a reference to string data which should live perpetually, and the source string is always truncated, while performing no copies in the situations where we can get away with it. I like this as it provides very predictable behavior which is also performant. I imagine it would be very confusing and dangerous to copy dependent string buffer references when using a move operator, and we definitely don't want to share a reference into a fixed string buffer, as it will be likely to go away or mutate.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8908186 -
Flags: review?(nfroyd)
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8908187 -
Flags: review?(nfroyd)
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8908188 -
Flags: review?(nfroyd)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → michael
Comment 4•7 years ago
|
||
Comment on attachment 8908186 [details] [diff] [review] Part 1: Add move overload to nsA[C]String::Assign Review of attachment 8908186 [details] [diff] [review]: ----------------------------------------------------------------- These patches need some tests, preferably testing a couple different variations: {owned, shared, auto, etc} x {owned, shared, auto, etc}. I'm not completely sure how to test some things, since we might have static analyses that ensure that you don't touch moved-from things after moving from them? I guess there would be some testing from people returning ns*String from functions, which would automatically start using move constructors, so maybe UAF and leaks would get checked-for by ASan etc. Regardless, having standalone tests for this sort of stuff would be good to make sure that we really understand the entire space here.
Attachment #8908186 -
Flags: review?(nfroyd)
Comment 5•7 years ago
|
||
Comment on attachment 8908187 [details] [diff] [review] Part 2: Add move overloads to nsA[C]String assignment and constructors Review of attachment 8908187 [details] [diff] [review]: ----------------------------------------------------------------- Clearing for tests from comment 1.
Attachment #8908187 -
Flags: review?(nfroyd)
Comment 6•7 years ago
|
||
Comment on attachment 8908188 [details] [diff] [review] Part 3: Expose nsA[C]String::Assign(nsA[C]String&&) overload as take_from to rust Review of attachment 8908188 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/rust/nsstring/src/lib.rs @@ +304,5 @@ > + unsafe { $take_from(self, other) }; > + } > + > + /// Take the value of `other` and set `self`, overwriting any value > + /// currently stored. The passed-in string will be truncated. This is the ideal state of things if we return Ok(); what happens if Err() is returned?
Attachment #8908188 -
Flags: review?(nfroyd)
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #6) > ::: xpcom/rust/nsstring/src/lib.rs > @@ +304,5 @@ > > + unsafe { $take_from(self, other) }; > > + } > > + > > + /// Take the value of `other` and set `self`, overwriting any value > > + /// currently stored. The passed-in string will be truncated. > > This is the ideal state of things if we return Ok(); what happens if Err() > is returned? The code currently unconditionally truncates the source string unless &source == &dest, so if Err() is returned then the assignment failed but the source string was destroyed. Perhaps we want to not destroy the source string in that situation.
Assignee | ||
Comment 8•7 years ago
|
||
This test passes locally with debug builds for me. I think it tests the interesting cases decently well. In addition, I'm fairly confident that the logic in ::Assign(nsA[C]String&&) is correct, as there's only one branch (source is either OWNED or SHARED) which is different than the normal Assign path. All other types perform a normal copy-assign followed by a `Truncate`.
Attachment #8908318 -
Flags: review?(nfroyd)
Assignee | ||
Updated•7 years ago
|
Attachment #8908186 -
Flags: review?(nfroyd)
Assignee | ||
Updated•7 years ago
|
Attachment #8908187 -
Flags: review?(nfroyd)
Assignee | ||
Updated•7 years ago
|
Attachment #8908188 -
Flags: review?(nfroyd)
Comment 9•7 years ago
|
||
Comment on attachment 8908318 [details] [diff] [review] Part 4: Add tests for nsA[C]String::Assign move overload Review of attachment 8908318 [details] [diff] [review]: ----------------------------------------------------------------- These are probably not bad tests for all manner of string functionality, not just move operations. We should have some tests for moving from actual substrings (probably nsDependent*Substring) to ensure that the non-TERMINATED case works correctly. (It should, since substrings should not be SHARED or OWNED, but "trust, but verify".) ::: xpcom/tests/gtest/TestMoveString.cpp @@ +31,5 @@ > + EXPECT_EQ(aStr.GetDataFlags(), Df::OWNED | Df::TERMINATED); > + EXPECT_STREQ(aStr.BeginReading(), aValue); > +} > + > +void ExpectTerminated(const nsACString& aStr) Maybe this should be ExpectEmptyString? I guess by "Terminated" you were going for the "dead" meaning rather than the ns*String "ends in a null" meaning?
Attachment #8908318 -
Flags: review?(nfroyd) → feedback+
Comment 10•7 years ago
|
||
Comment on attachment 8908186 [details] [diff] [review] Part 1: Add move overload to nsA[C]String::Assign Review of attachment 8908186 [details] [diff] [review]: ----------------------------------------------------------------- I feel better about this patch now that we're going to have tests.
Attachment #8908186 -
Flags: review?(nfroyd) → review+
Updated•7 years ago
|
Attachment #8908187 -
Flags: review?(nfroyd) → review+
Comment 11•7 years ago
|
||
Comment on attachment 8908188 [details] [diff] [review] Part 3: Expose nsA[C]String::Assign(nsA[C]String&&) overload as take_from to rust Review of attachment 8908188 [details] [diff] [review]: ----------------------------------------------------------------- I think we want to try and not destroy the source string here; can you see if that behavior is reasonable, and if so, add tests for it (Rust or C, your choice)?
Attachment #8908188 -
Flags: review?(nfroyd) → feedback+
Assignee | ||
Comment 12•7 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #9) > These are probably not bad tests for all manner of string functionality, not > just move operations. Are you suggesting I copy these tests and write a non-moving version of them? > We should have some tests for moving from actual substrings (probably > nsDependent*Substring) to ensure that the non-TERMINATED case works > correctly. (It should, since substrings should not be SHARED or OWNED, but > "trust, but verify".) The nsDependentCSubstring method does try to use an actual substring (I explicitly use the char*/length constructor for nsDependentCSubstring, and confirm that the source doesn't have the TERMINATED flag set). The source string _does_ actually have a terminator though - I'll try adding some garbage on the end to make sure it works well. > ::: xpcom/tests/gtest/TestMoveString.cpp > @@ +31,5 @@ > > + EXPECT_EQ(aStr.GetDataFlags(), Df::OWNED | Df::TERMINATED); > > + EXPECT_STREQ(aStr.BeginReading(), aValue); > > +} > > + > > +void ExpectTerminated(const nsACString& aStr) > > Maybe this should be ExpectEmptyString? I guess by "Terminated" you were > going for the "dead" meaning rather than the ns*String "ends in a null" > meaning? I meant to write ExpectTruncated, and apparently just... wrote something completely different? - (I wanted to confirm that the string was in the same state that it would be after calling Truncate()). I'm going to stick with that name unless you prefer ExpectEmptyString. (In reply to Nathan Froyd [:froydnj] from comment #11) > I think we want to try and not destroy the source string here; can you see > if that behavior is reasonable, and if so, add tests for it (Rust or C, your > choice)? I'm not sure how to cause the string allocation to fail to test this behaviour :-/. Do we have a way to force allocations to fail for testing purposes?
Assignee | ||
Comment 13•7 years ago
|
||
GCC 4.9 has some problems resolving expressions like the following when adding a new operator=(T&&) overload to nsTSubstring: aOutAString = NS_LITERAL_STRING("..."); This appears to be due to a frontend bug which means that adding another operator= overload causes some coersions to not occur implicitly. Namely the expression above ends up generating implicit calls like: (nsTSubstring& aOutAString)::operator=( nsTSubstring&& rvalue_ref_temporary( nsTSubstring nsTSubstring( // << This constructor is protected const nsTSubstring& derive_to_base( operator const nsTString&( const nsTLiteralString& ...))))) Which causes a failure, as the nsTSubstring copy constructor is protected, and we could instead use a different overload of operator= (which is used in more recent versions of gcc): (nsTSubstring& aOutAString)::operator=( const nsTSubstring& derive_to_base( operator const nsTString&( const nsTLiteralString& ...))) This change means that the implicit cast operator is no longer necessary, and we can use the following coercion which doesn't mess up on gcc 4.9: (nsTSubstring& aOutAString)::operator=( const nsTSubstring& derive_to_base( const nsTLiteralString& ...))
Attachment #8911940 -
Flags: review?(nfroyd)
Assignee | ||
Comment 14•7 years ago
|
||
I changed the behaviour here to match what you asked for in comment 11.
Attachment #8912279 -
Flags: review?(nfroyd)
Assignee | ||
Updated•7 years ago
|
Attachment #8908186 -
Attachment is obsolete: true
Attachment #8908187 -
Attachment is obsolete: true
Attachment #8908188 -
Attachment is obsolete: true
Attachment #8908318 -
Attachment is obsolete: true
Assignee | ||
Comment 15•7 years ago
|
||
Assignee | ||
Comment 16•7 years ago
|
||
I changed the docs here to match the change in part 1
Attachment #8912281 -
Flags: review?(nfroyd)
Assignee | ||
Comment 17•7 years ago
|
||
I changed the tests a bit like you asked, but I'm not sure if a) You want me to generalize these tests to also test non-moving assignments, and b) How to test allocation failure cases. I'd love some input on how to test those, so I'm requesting feedback.
Attachment #8912282 -
Flags: feedback?(nfroyd)
Comment 18•7 years ago
|
||
Comment on attachment 8912279 [details] [diff] [review] Part 1: Add move overload to nsA[C]String::Assign Review of attachment 8912279 [details] [diff] [review]: ----------------------------------------------------------------- Thank you!
Attachment #8912279 -
Flags: review?(nfroyd) → review+
Comment 19•7 years ago
|
||
Comment on attachment 8912282 [details] [diff] [review] Part 4: Add tests for nsA[C]String::Assign move overload Review of attachment 8912282 [details] [diff] [review]: ----------------------------------------------------------------- Testing just the moving assignments is OK, thanks. Allocation failure cases are hard. We don't really have anything good for that on the Gecko side of things besides "allocate something really big on 32-bit platforms". =/ If ns*String took an AllocPolicy parameter, our tests could sub in an OOM-simulating one, but that's a pretty big job...
Attachment #8912282 -
Flags: feedback?(nfroyd) → feedback+
Comment 20•7 years ago
|
||
Comment on attachment 8911940 [details] [diff] [review] Part 5: Make nsTLiteralString inherit from nsTSubstring instead of nsTStringRepr Review of attachment 8911940 [details] [diff] [review]: ----------------------------------------------------------------- I'm not super excited about this patch, since this just gets us back to the pre-bug 1344629 state of affairs, and looks like it reintroduces some static constructors while doing so. Could we just alter the (ideally few) places that are using assignment from literal string temporaries to use aOutAString.AssignLiteral(...), which is what they should have been using anyway? ::: xpcom/string/nsTLiteralString.h @@ -17,5 @@ > * nsTString-lookalike that restricts its string value to a literal character > * sequence. Can be implicitly cast to const nsTString& (the const is > * essential, since this class's data are not writable). The data are assumed > * to be static (permanent) and therefore, as an optimization, this class > * does not have a destructor. This comment would no longer be true if this patch lands.
Attachment #8911940 -
Flags: review?(nfroyd) → review-
Comment 21•7 years ago
|
||
Comment on attachment 8912281 [details] [diff] [review] Part 3: Expose nsA[C]String::Assign(nsA[C]String&&) overload as take_from to rust Review of attachment 8912281 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/rust/nsstring/src/lib.rs @@ +305,5 @@ > + } > + > + /// Take the value of `other` and set `self`, overwriting any value > + /// currently stored. If this function fails, the source string will > + /// be left untouched, otherwise it will be truncated. This comment seems confusing to me: hiding the truncation-on-success at the end of the second sentence reads strangely. Maybe: Take the value of `other` and set `self`, overwriting any value currently stored. The source string will be truncated on success. If this function fails (e.g. a copy of the source string can not be made), the source string will be left untouched.
Attachment #8912281 -
Flags: review?(nfroyd) → review+
Comment 22•7 years ago
|
||
Also note that I was looking at the static constructor numbers on Linux, whereas the motivating example for bug 1344629 was static constructors on Windows. So the situation from p5 on Windows might actually be worse.
Assignee | ||
Comment 23•7 years ago
|
||
This is going to be a pain to fix in a non-undefined-behaviour-filled way with GCC 4.9, so I'm inclined to leave it until then.
Assignee | ||
Comment 24•7 years ago
|
||
I had some time while waiting for try pushes to try this out again, and I think I got a workaround which works. MozReview-Commit-ID: CHywpZ4fvXf
Attachment #8915749 -
Flags: review?(nfroyd)
Assignee | ||
Updated•7 years ago
|
Attachment #8911940 -
Attachment is obsolete: true
Comment 25•7 years ago
|
||
Comment on attachment 8915749 [details] [diff] [review] Part 5: Add a workaround for gcc 4.9 compiler bug Review of attachment 8915749 [details] [diff] [review]: ----------------------------------------------------------------- This works for me, and is less terrible than previous versions. ::: xpcom/string/nsTString.h @@ +126,5 @@ > } > > + // NOTE(nika): gcc 4.9 workaround. Remove when support is dropped. > + explicit > + nsTString(const literalstring_type& aReadable) Nit: This is sort of peculiar formatting; I think the usual would be putting this all on one line: explicit nsTString(const literalstring_type& aReadable) @@ +663,5 @@ > } > > + // NOTE(nika): gcc 4.9 workaround. Remove when support is dropped. > + explicit > + nsTAutoStringN(const literalstring_type& aStr) Same nit with the formatting here.
Attachment #8915749 -
Flags: review?(nfroyd) → review+
Comment 26•7 years ago
|
||
Pushed by michael@thelayzells.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bf17a82ef4a9 Part 1: Add move overload to nsA[C]String::Assign, r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/4b8f494ccca3 Part 2: Add move overloads to nsA[C]String assignment and constructors, r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/2f081c828265 Part 3: Expose nsA[C]String::Assign(nsA[C]String&&) overload as take_from to rust, r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/1aa544eccfc2 Part 4: Add tests for nsA[C]String::Assign move overload, r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/6206e82fc670 Part 5: Add a workaround for gcc 4.9 compiler bug, r=froydnj
Comment 27•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bf17a82ef4a9 https://hg.mozilla.org/mozilla-central/rev/4b8f494ccca3 https://hg.mozilla.org/mozilla-central/rev/2f081c828265 https://hg.mozilla.org/mozilla-central/rev/1aa544eccfc2 https://hg.mozilla.org/mozilla-central/rev/6206e82fc670
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•