Closed Bug 1453990 Opened 2 years ago Closed 2 years ago

-Wreturn-std-move: incorrect warning on nsAutoCString

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: Sylvestre, Assigned: andi)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file)

In file included from /var/lib/jenkins/workspace/firefox-clang-last/obj-x86_64-pc-linux-gnu/xpcom/threads/Unified_cpp_xpcom_threads0.cpp:137:
/var/lib/jenkins/workspace/firefox-clang-last/xpcom/threads/Scheduler.cpp:793:10: error: local variable 'result' will be copied despite being returned by name [-Werror,-Wreturn-std-move]
  return result;
         ^~~~~~
/var/lib/jenkins/workspace/firefox-clang-last/xpcom/threads/Scheduler.cpp:793:10: note: call 'std::move' explicitly to avoid copying
  return result;
         ^~~~~~
         std::move(result)


Triggered by the clang change upstream: https://reviews.llvm.org/rC329914
(landed yesterday)
That's an interesting warning. In this case I don't think we actually want to move the string [1], we want a copy since nsPrintfCString is really an nsAutoCString [2].

[1] https://searchfox.org/mozilla-central/rev/9f3da81290054c5b8955bb67ff98cae66676f745/xpcom/threads/Scheduler.cpp#783-795
[2] https://searchfox.org/mozilla-central/rev/9f3da81290054c5b8955bb67ff98cae66676f745/xpcom/string/nsPrintfCString.h#23
Eric, is that a false positive or an issue on our side?
Assignee: nobody → bpostelnicu
We have a bunch of occurrences of this pattern in this code base (probably 10+)
Summary: -Wreturn-std-move on xpcom/threads/Scheduler.cpp → -Wreturn-std-move: incorrect warning on nsAutoCString
See Also: → 1454848
Duplicate of this bug: 1454848
Comment on attachment 8975240 [details]
Bug 1453990 - Disable -Werror on -Wreturn-std-move. We have a false positive with nsAutoCString

https://reviewboard.mozilla.org/r/243586/#review249606
Attachment #8975240 - Flags: review?(nfroyd) → review+
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0e8e2c4c09e2
Disable -Werror on -Wreturn-std-move. We have a false positive with nsAutoCString r=froydnj
https://hg.mozilla.org/mozilla-central/rev/0e8e2c4c09e2
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
I saw this on clang-cl build, and I don't think this is a false positive actually.

In these cases, we are constructing nsAutoCString, but returning them as an nsCString. This will cause additional allocation and string copy because nsCString cannot hold stack-allocated buffer from nsAutoCString.

The right fix here is to change the stack type from nsAutoCString to just nsCString, and the move would happen automatically.
I think we should back out the suppression and fix most of them. nsPrintfCString is probably not fixable, but other nsAutoCString should be replaced with nsCString if they are returned as the latter.
See Also: → 1476477
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #9)
> I saw this on clang-cl build, and I don't think this is a false positive
> actually.

It's still a false positive, we don't want to move nsAutoStrings.

> In these cases, we are constructing nsAutoCString, but returning them as an
> nsCString. This will cause additional allocation and string copy because
> nsCString cannot hold stack-allocated buffer from nsAutoCString.

Yes, in the hasnt-overrun-stack-buffer case. There are potentially some cases where we want to avoid realloc's while building a string, but are okay with alloc-and-copy on return.

> The right fix here is to change the stack type from nsAutoCString to just
> nsCString, and the move would happen automatically.

Sure, that's going to make sense most of the time.
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #11)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #9)
> > I saw this on clang-cl build, and I don't think this is a false positive
> > actually.
> 
> It's still a false positive, we don't want to move nsAutoStrings.

Why?

> > In these cases, we are constructing nsAutoCString, but returning them as an
> > nsCString. This will cause additional allocation and string copy because
> > nsCString cannot hold stack-allocated buffer from nsAutoCString.
> 
> Yes, in the hasnt-overrun-stack-buffer case. There are potentially some
> cases where we want to avoid realloc's while building a string, but are okay
> with alloc-and-copy on return.

In that case, we should move nsAutoStrings, because otherwise if we end up allocating in nsAutoString, the buffer can be moved to the returned value without refcount changes.
Eric, what do you think about comment #12?
Flags: needinfo?(erahm)

(In reply to Sylvestre Ledru [:sylvestre] from comment #13)

Eric, what do you think about comment #12?

Yeah that sounds right, we added proper move support back in bug 1377351.

Flags: needinfo?(erahm)
You need to log in before you can comment on or make changes to this bug.