1453990 - -Wreturn-std-move: incorrect warning on nsAutoCString

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

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)

Comment 1

[Eric Rahm [:erahm]]

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

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Eric, is that a false positive or an issue on our side?

Comment 3

[Sylvestre Ledru [:Sylvestre]]

We have a bunch of occurrences of this pattern in this code base (probably 10+)

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1453990 - Disable -Werror on -Wreturn-std-move. We have a false positive with nsAutoCString

See: https://bugs.llvm.org/show_bug.cgi?id=37249

Review commit: https://reviewboard.mozilla.org/r/243586/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243586/

Comment 6

[Nathan Froyd [:froydnj]]

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e8e2c4c09e2
Disable -Werror on -Wreturn-std-move. We have a false positive with nsAutoCString r=froydnj

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/0e8e2c4c09e2

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 11

[Eric Rahm [:erahm]]

(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.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

(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.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Eric, what do you think about comment #12?

Comment 14

[Eric Rahm [: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.