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
Updated•15 years ago
|
Assignee: nobody → toshiba.ctr
Comment 3•15 years ago
|
||
Comment 4•15 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•15 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•15 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•15 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•15 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•15 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? :)
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
•