This is an archive of the discontinued LLVM Phabricator instance.

[NFC][CLANG] Fix coverity remarks about large copy by values
ClosedPublic

Authored by Manna on Apr 25 2023, 9:49 AM.

Details

Summary

Reported By Static Analyzer Tool, Coverity:

Big parameter passed by value
Copying large values is inefficient, consider passing by reference; Low, medium, and high size thresholds for detection can be adjusted.

  1. Inside "CodeGenModule.cpp" file, in clang::​CodeGen::​CodeGenModule::​EmitBackendOptionsMetadata(clang::​CodeGenOptions): A very large function call parameter exceeding the high threshold is passed by value.

pass_by_value: Passing parameter CodeGenOpts of type clang::CodeGenOptions const (size 2168 bytes) by value, which exceeds the high threshold of 512 bytes.

  1. Inside "SemaType.cpp" file, in IsNoDerefableChunk(clang::​DeclaratorChunk): A large function call parameter exceeding the low threshold is passed by value.

pass_by_value: Passing parameter Chunk of type clang::DeclaratorChunk (size 176 bytes) by value, which exceeds the low threshold of 128 bytes.

  1. Inside "CGNonTrivialStruct.cpp" file, in <unnamed>::​getParamAddrs<2ul, <0ul, 1ul...>>(std::​integer_sequence<unsigned long, T2...>, std::​array<clang::​CharUnits, T1>, clang::​CodeGen::​FunctionArgList, clang::​CodeGen::​CodeGenFunction *): A large function call parameter exceeding the low threshold is passed by value.

pass_by_value: Passing parameter Args of type clang::CodeGen::FunctionArgList (size 144 bytes) by value, which exceeds the low threshold of 128 bytes.

  1. Inside "CGGPUBuiltin.cpp" file, in <unnamed>::​containsNonScalarVarargs(clang::​CodeGen::​CodeGenFunction *, clang::​CodeGen::​CallArgList): A very large function call parameter exceeding the high threshold is passed by value.

pass_by_value: Passing parameter Args of type clang::CodeGen::CallArgList (size 1176 bytes) by value, which exceeds the high threshold of 512 bytes.

Diff Detail

Event Timeline

Manna created this revision.Apr 25 2023, 9:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2023, 9:49 AM
Herald added a subscriber: arphaman. · View Herald Transcript
Manna requested review of this revision.Apr 25 2023, 9:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2023, 9:49 AM
Manna updated this revision to Diff 516834.Apr 25 2023, 10:01 AM
Manna edited the summary of this revision. (Show Details)Apr 25 2023, 10:05 AM
Manna updated this revision to Diff 516883.Apr 25 2023, 1:26 PM

Fix bot failures

Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2023, 1:26 PM
Herald added a subscriber: kadircet. · View Herald Transcript
Manna updated this revision to Diff 517010.Apr 25 2023, 6:29 PM
Manna updated this revision to Diff 517018.Apr 25 2023, 7:05 PM
Manna edited the summary of this revision. (Show Details)
tahonermann accepted this revision.Apr 27 2023, 2:54 PM

These changes all look good to me. For the DeclaratorChunk case, I noted an additional removal of a comment that I think should be considered. Assuming I haven't misread the code, DeclaratorChunk does not look like a small value type (at least not any more).

clang/lib/CodeGen/CGGPUBuiltin.cpp
128

This looks like a good change; CallArgList derives from SmallVector<CallArg, 8> and holds a few other SmallVector specializations as data members.

clang/lib/CodeGen/CGNonTrivialStruct.cpp
326

This looks like a good change. FunctionArgList derives from SmallVector<const VarDecl *, 16>.

clang/lib/CodeGen/CodeGenModule.h
1714

This is definitely a good change; it seems likely that the omission of the & was not intentional.

clang/lib/Sema/SemaType.cpp
4556

The definition of DeclaratorChunk has this comment:

/// This is intended to be a small value object.

However, it contains what looks likely to be a sizable anonymous union. See clang/include/clang/Sema/DeclSpec.h lines 1587-1595 and the definition of FunctionTypeInfo in that same file. I don't see other pass-by-value uses of this type. Perhaps the above comment should be removed.

This revision is now accepted and ready to land.Apr 27 2023, 2:54 PM
Manna marked 4 inline comments as done.Apr 28 2023, 5:58 AM
Manna added inline comments.
clang/lib/CodeGen/CGGPUBuiltin.cpp
128

CallArgList derives from SmallVector<CallArg, 8> and holds a few other SmallVector specializations as data members.

Yes

clang/lib/CodeGen/CodeGenModule.h
1714

Agreed

clang/lib/Sema/SemaType.cpp
4556

Thanks @tahonermann for reviews and feedbacks.

See clang/include/clang/Sema/DeclSpec.h lines 1587-1595 and the definition of FunctionTypeInfo in that same file. I don't see other pass-by-value uses of this type.

Yes, this is what i saw in same file (https://clang.llvm.org/doxygen/SemaType_8cpp_source.html) while investigating this bug - it is usually passing by const references.

This revision was automatically updated to reflect the committed changes.
Manna marked 3 inline comments as done.