Open
Bug 500864
Opened 15 years ago
Updated 1 year ago
[meta] Warn on passing large objects by value
Categories
(Developer Infrastructure :: Source Code Analysis, enhancement, P3)
Developer Infrastructure
Source Code Analysis
Tracking
(Not tracked)
NEW
People
(Reporter: jruderman, Unassigned)
References
(Depends on 2 open bugs, Blocks 1 open bug)
Details
(Keywords: meta, perf)
Attachments
(3 files)
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."
Updated•15 years ago
|
Keywords: student-project
Comment 1•15 years ago
|
||
I am a student from SJCE, Mysore India and would like to take up this bug.
Updated•15 years ago
|
Assignee: nobody → toshiba.ctr
Comment 2•15 years ago
|
||
This is not a patch but just a first cut.
Comment 3•14 years ago
|
||
Comment 4•14 years ago
|
||
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•14 years ago
|
||
(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•14 years ago
|
||
(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•14 years ago
|
||
(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•14 years ago
|
||
(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•14 years ago
|
||
(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?
Updated•7 years ago
|
Product: Core → Firefox Build System
Comment 10•6 years ago
|
||
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
Assignee: toshiba.ctr → nobody
OS: Mac OS X → Unspecified
Priority: -- → P3
Hardware: x86 → Unspecified
Comment 11•6 years ago
|
||
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
Assignee: nobody → sledru
Updated•6 years ago
|
Blocks: static-analyzers
Keywords: student-project
Updated•6 years ago
|
Comment hidden (mozreview-request) |
Updated•6 years ago
|
See Also: → https://bugs.llvm.org/show_bug.cgi?id=38175
Comment 13•6 years ago
|
||
mozreview-review |
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?
Attachment #8992666 -
Flags: review?(nfroyd) → review+
Comment 14•6 years ago
|
||
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•6 years ago
|
||
Deal, thank you for looking at things!
Updated•2 years ago
|
Product: Firefox Build System → Developer Infrastructure
Updated•2 years ago
|
Severity: normal → S3
Updated•1 year ago
|
Assignee: sledru → nobody
Keywords: meta
Summary: Warn on passing large objects by value → [meta] Warn on passing large objects by value
You need to log in
before you can comment on or make changes to this bug.
Description
•