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)

enhancement

Tracking

(Not tracked)

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."
Keywords: student-project
I am a student from SJCE, Mysore India and would like to take up this bug.
Assignee: nobody → toshiba.ctr
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
(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.
(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?
(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?
Blocks: 5416
(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.
(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?
Product: Core → Firefox Build System
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
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
Depends on: 1476307
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+
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? :)
Deal, thank you for looking at things!
Depends on: 1476673
Depends on: 1476675
Depends on: 1508225
Product: Firefox Build System → Developer Infrastructure
Severity: normal → S3
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.

Attachment

General

Creator:
Created:
Updated:
Size: