500864 - [meta] Warn on passing large objects by value

Status: NEW

(Developer Infrastructure :: Source Code Analysis, enhancement, P3)

Description

[Jesse Ruderman]

Would be nice to have an analysis to generate warnings like:

"Object of class nsFoo (80 bytes) passed by value. Consider passing by const ref instead."

Comment 1

[Toshiba T R]

I am a student from SJCE, Mysore India and would like to take up this bug.

Comment 2

[Toshiba T R]

Attachment: This JS warns if "class" or "struct " objects of size greater than 64 bytes is passed by value.Please look at this and give your opinion.

This is not a patch but just a first cut.

Comment 3

[Toshiba T R]

Attachment: This JS warns if "class" or "struct " objects of size greater than 64 bytes is passed by value.

Comment 4

[Toshiba T R]

Comment on attachment 444146 [details]
This JS warns if "class" or "struct " objects of size greater than 64 bytes is passed by value. 

Hi Taras,

Please give feedback.

Regards,
Toshiba

Comment 5

[(dormant account)]

(In reply to comment #4)
> (From update of attachment 444146 [details])
> Hi Taras,
> 
> Please give feedback.
> 
> Regards,
> Toshiba

Upload a list of pass-by-value locations this script finds in mozilla trunk.

Comment 6

[Toshiba T R]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 444146 [details] [details])
> > Hi Taras,
> > 
> > Please give feedback.
> > 
> > Regards,
> > Toshiba
> 
> Upload a list of pass-by-value locations this script finds in mozilla trunk.

Is it alright if warnings are generated for files in the tests folder also?

Comment 7

[Toshiba T R]

(In reply to comment #0)
> Would be nice to have an analysis to generate warnings like:
> 
> "Object of class nsFoo (80 bytes) passed by value. Consider passing by const
> ref instead."

May I know how this thought came?

Comment 8

[(dormant account)]

(In reply to comment #6)

> Is it alright if warnings are generated for files in the tests folder also?

please filter our irrelevant stuff like tests.

(In reply to comment #7)
> > "Object of class nsFoo (80 bytes) passed by value. Consider passing by const
> > ref instead."
> 
> May I know how this thought came?

I'm not sure what you mean. Point of this analysis is to avoid needless memcpy()ing.

Comment 9

[Toshiba T R]

(In reply to comment #8)
> (In reply to comment #6)
> 
> > Is it alright if warnings are generated for files in the tests folder also?
> 
> please filter our irrelevant stuff like tests.
> 
> (In reply to comment #7)
> > > "Object of class nsFoo (80 bytes) passed by value. Consider passing by const
> > > ref instead."
> > 
> > May I know how this thought came?
> 
> I'm not sure what you mean. Point of this analysis is to avoid needless
> memcpy()ing.

I meant are there  any pass-by-values of size  64 bytes above in mozilla trunk?
I got only few for 36 bytes and 20 bytes and many for 12 bytes and 8 bytes?

Comment 10

[Chris Peterson [:cpeterson]]

clang has a -Wlarge-by-value-copy warning, enabled by default, though I don't know the default permitted size.

> warning: A is a large (B bytes) pass-by-value argument; pass it by reference instead ?
> warning: return value of A is a large (B bytes) pass-by-value object; pass it by reference instead ?

https://clang.llvm.org/docs/DiagnosticsReference.html#wlarge-by-value-copy

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Chris, reading at the source of clang [1], it seems that -Wlarge-by-value-copy is only useful when a value > 0 is provided.
By default, the value is 0 and the function returns.

I did a run with 512 as value and only two warnings are found (in Skia).
I will see how to enable it by default for real.

By the way, https://clang.llvm.org/docs/DiagnosticsReference.html#wlarge-by-value-copy is a very poor documentation :'(
(autogenerated without much capability to update the doc).
I reported https://bugs.llvm.org/show_bug.cgi?id=38175 upstream for this.

[1] https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaDecl.cpp#L12081

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 500864 - Enable clang warning -Wlarge-by-value-copy. Value set to 256 for now

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

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 8992666 [details]
Bug 500864 - Enable clang warning -Wlarge-by-value-copy. Value set to 256 for now

https://reviewboard.mozilla.org/r/257524/#review264410

Thanks, this seems potentially useful.

::: commit-message-3aca1:1
(Diff revision 1)
> +Bug 500864 - Enable clang warning -Wlarge-by-value-copy. Value set to 256 for now r?froydnj

I think the "Value set to 256 for now" is unnecessary here.

::: build/moz.configure/warnings.configure:113
(Diff revision 1)
>  
>  # Disable the -Werror for -Wclass-memaccess as we have a long
>  # tail of issues to fix
>  check_and_add_gcc_warning('-Wno-error=class-memaccess')
>  
> +# Check if the argument should not be pass by reference instead of copy

Nit: "should be passed by reference", I think is the phrasing desired here.

::: build/moz.configure/warnings.configure:114
(Diff revision 1)
>  # Disable the -Werror for -Wclass-memaccess as we have a long
>  # tail of issues to fix
>  check_and_add_gcc_warning('-Wno-error=class-memaccess')
>  
> +# Check if the argument should not be pass by reference instead of copy
> +# The value has been chosen arbitrary.

Nit: "arbitrarily"

::: build/moz.configure/warnings.configure:115
(Diff revision 1)
>  # tail of issues to fix
>  check_and_add_gcc_warning('-Wno-error=class-memaccess')
>  
> +# Check if the argument should not be pass by reference instead of copy
> +# The value has been chosen arbitrary.
> +check_and_add_gcc_warning('-Wlarge-by-value-copy=256')

Is it possible to reduce this further (192?  128?)?  As a follow-up, perhaps?

Comment 14

[Sylvestre Ledru [:Sylvestre]]

I tried with 128.
It found:
VRDisplayClient.cpp:272, Clang (LLVM based), Priority: Normal
return value of 'GetSensorState' is a large (224 bytes) pass-by-value object; pass it by reference instead ?

gfxVRExternal.cpp:96, Clang (LLVM based), Priority: Normal
return value of 'GetSensorState' is a large (224 bytes) pass-by-value object; pass it by reference instead ?

gfxVROSVR.cpp:300, Clang (LLVM based), Priority: Normal
return value of 'GetSensorState' is a large (224 bytes) pass-by-value object; pass it by reference instead ?

gfxVROpenVR.cpp:251, Clang (LLVM based), Priority: Normal
return value of 'GetSensorState' is a large (224 bytes) pass-by-value object; pass it by reference instead ?

gfxVRPuppet.cpp:154, Clang (LLVM based), Priority: Normal
return value of 'GetSensorState' is a large (224 bytes) pass-by-value object; pass it by reference instead ?

nsTextFrame.cpp:1768, Clang (LLVM based), Priority: Normal
return value of 'GetFirstFontMetrics' is a large (144 bytes) pass-by-value object; pass it by reference instead ?

+ bug 1476307

+ 2 in skia 
+ 1 in webrtc

I propose that we land 256 when bug 1476307 is fixed. 
Then report bugs for the 6 above and move to 128.
Deal? :)

Comment 15

[Nathan Froyd [:froydnj]]

Deal, thank you for looking at things!