This is an archive of the discontinued LLVM Phabricator instance.

Fix std::has_unique_object_representations for _BitInt types with padding bits
ClosedPublic

Authored by MitalAshok on May 17 2022, 9:00 AM.

Details

Summary

"std::has_unique_object_representations<_BitInt(N)>" was always true, even if the type has padding bits (since the trait assumes all integer types have no padding bits). The standard has an explicit note that this should not hold for types with padding bits.

Diff Detail

Event Timeline

MitalAshok created this revision.May 17 2022, 9:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2022, 9:00 AM
MitalAshok retitled this revision from Fix std::has_unique_object_representations for _BitInt types with padding to Fix std::has_unique_object_representations for _BitInt types with padding bits.May 17 2022, 9:08 AM
MitalAshok edited the summary of this revision. (Show Details)
MitalAshok edited projects, added Restricted Project; removed Restricted Project.
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2022, 9:17 AM
MitalAshok published this revision for review.May 17 2022, 9:19 AM

I think I agree with the justification here, though am a touch confused in the test. I'm also a touch concerned that we have getSubobjectSizeInBits returning a 'rounded up to power of 2' bit happening here. The bitfield case returns non-powers-of-two, but the _BitInt case does not.

clang/lib/AST/ASTContext.cpp
2696

This answer ends up being wrong in the case of _BitInt, consider _BitInt(7). Its field-size would be 7, yet this would result in 8. I'm not sure of the fallout of this though.

clang/test/SemaCXX/has_unique_object_reps_bitint.cpp
8

Whats going on here? I don't particularly get the condition.

MitalAshok added inline comments.May 17 2022, 9:42 AM
clang/lib/AST/ASTContext.cpp
2696

The field size should be 8 for _BitInt(7), since it takes 8 bits in the struct (including the padding bit). It is filtered out later with "hasUniqueObjectRepresentations" on line 2709 if it wasn't in a bit field

clang/test/SemaCXX/has_unique_object_reps_bitint.cpp
8

sizeof(_BitInt(24)) == sizeof(int32_t) (4) since we align to the next larger builtin integer type. But if that weren't the case for some reason (i.e., that changes in the future, I don't know if _BitInt is ABI stable), this wouldn't start failing for unrelated reasons

This likely also needs a Release Note.

clang/lib/AST/ASTContext.cpp
2696

I see, thanks for the explanation.

clang/test/SemaCXX/has_unique_object_reps_bitint.cpp
8

I see! Can you clarify that with a comment here?

MitalAshok edited the summary of this revision. (Show Details)

Added release note and missing test for struct with reference member

erichkeane accepted this revision.May 18 2022, 5:54 AM
This revision is now accepted and ready to land.May 18 2022, 5:54 AM
aaron.ballman added inline comments.May 18 2022, 5:56 AM
clang/lib/AST/ASTContext.cpp
2689
2690–2691

A type can't be both a _BitInt and a reference at the same time, so I think you can drop the !reference type check here.

2808

Removed useless isReferenceType check

MitalAshok marked 7 inline comments as done.May 18 2022, 7:22 AM

Rebase release notes

aaron.ballman accepted this revision.May 28 2022, 4:59 AM

LGTM as well -- do you need someone to commit on your behalf? If so, what name and email address would you like used for patch attribution?

@aaron.ballman yes, with the author "Mital Ashok <mital.vaja[AT]googlemail.com>". (replacing the [AT]) Thanks!

browneee added inline comments.
clang/test/SemaCXX/has_unique_object_reps_bitint.cpp
1

This unit test appears to trigger a memory leak:

https://lab.llvm.org/buildbot/#/builders/5/builds/24359/steps/15/logs/stdio

FAIL: Clang :: SemaCXX/has_unique_object_reps_bitint.cpp (13615 of 66177)
******************** TEST 'Clang :: SemaCXX/has_unique_object_reps_bitint.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   /b/sanitizer-x86_64-linux-fast/build/llvm_build_asan/bin/clang -cc1 -internal-isystem /b/sanitizer-x86_64-linux-fast/build/llvm_build_asan/lib/clang/15.0.0/include -nostdsysteminc -triple x86_64-unknown-unknown -fsyntax-only -verify -std=c++17 -Wno-bitfield-width /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/test/SemaCXX/has_unique_object_reps_bitint.cpp
--
Exit Code: 1
Command Output (stderr):
--
=================================================================
==22556==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 1632 byte(s) in 3 object(s) allocated from:
    #0 0x55cc84e8d6be in malloc /b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_malloc_linux.cpp:69:3
    #1 0x55cc8c0a417c in safe_malloc /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/Support/MemAlloc.h:26:18
    #2 0x55cc8c0a417c in llvm::SmallVectorBase<unsigned int>::grow_pod(void*, unsigned long, unsigned long) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Support/SmallVector.cpp:126:15
    #3 0x55cc94342e34 in grow_pod /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/ADT/SmallVector.h:120:11
    #4 0x55cc94342e34 in grow /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/ADT/SmallVector.h:506:41
    #5 0x55cc94342e34 in reserveForParamAndGetAddressImpl<llvm::SmallVectorTemplateBase<clang::TemplateArgumentLoc, true> > /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/ADT/SmallVector.h:225:11
    #6 0x55cc94342e34 in reserveForParamAndGetAddress /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/ADT/SmallVector.h:511:12
    #7 0x55cc94342e34 in push_back /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/ADT/SmallVector.h:543:23
    #8 0x55cc94342e34 in addArgument /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/include/clang/AST/TemplateBase.h:604:15
    #9 0x55cc94342e34 in clang::VarTemplateSpecializationDecl::setTemplateArgsInfo(clang::TemplateArgumentListInfo const&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/AST/DeclTemplate.cpp:1341:22
    #10 0x55cc937d5b66 in clang::TemplateDeclInstantiator::VisitVarTemplateSpecializationDecl(clang::VarTemplateDecl*, clang::VarDecl*, clang::TemplateArgumentListInfo const&, llvm::ArrayRef<clang::TemplateArgument>, clang::VarTemplateSpecializationDecl*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:3860:8
    #11 0x55cc937e1dd4 in clang::Sema::BuildVarTemplateInstantiation(clang::VarTemplateDecl*, clang::VarDecl*, clang::TemplateArgumentList const&, clang::TemplateArgumentListInfo const&, llvm::SmallVectorImpl<clang::TemplateArgument>&, clang::SourceLocation, llvm::SmallVector<clang::Sema::LateInstantiatedAttribute, 16u>*, clang::LocalInstantiationScope*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:5135:20
    #12 0x55cc933d777d in clang::Sema::CheckVarTemplateId(clang::VarTemplateDecl*, clang::SourceLocation, clang::SourceLocation, clang::TemplateArgumentListInfo const&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Sema/SemaTemplate.cpp:4621:41
    #13 0x55cc933daddc in CheckVarTemplateId /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Sema/SemaTemplate.cpp:4657:21
    #14 0x55cc933daddc in clang::Sema::BuildTemplateIdExpr(clang::CXXScopeSpec const&, clang::SourceLocation, clang::LookupResult&, bool, clang::TemplateArgumentListInfo const*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Sema/SemaTemplate.cpp:4748:22
    #15 0x55cc928ffef3 in clang::Sema::ActOnIdExpression(clang::Scope*, clang::CXXScopeSpec&, clang::SourceLocation, clang::UnqualifiedId&, bool, bool, clang::CorrectionCandidateCallback*, bool, clang::Token*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Sema/SemaExpr.cpp:2728:12
    #16 0x55cc91d0e988 in clang::Parser::tryParseCXXIdExpression(clang::CXXScopeSpec&, bool, clang::Token&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseExprCXX.cpp:610:17
    #17 0x55cc91d124d9 in clang::Parser::ParseCXXIdExpression(bool) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseExprCXX.cpp:676:7
    #18 0x55cc91cdb510 in clang::Parser::ParseCastExpression(clang::Parser::CastParseKind, bool, bool&, clang::Parser::TypeCastState, bool, bool*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseExpr.cpp:1626:11
    #19 0x55cc91ce0476 in clang::Parser::ParseCastExpression(clang::Parser::CastParseKind, bool, bool&, clang::Parser::TypeCastState, bool, bool*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseExpr.cpp
    #20 0x55cc91cd0635 in clang::Parser::ParseCastExpression(clang::Parser::CastParseKind, bool, clang::Parser::TypeCastState, bool, bool*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseExpr.cpp:681:20
    #21 0x55cc91cd8160 in clang::Parser::ParseConstantExpressionInExprEvalContext(clang::Parser::TypeCastState) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseExpr.cpp:206:18
    #22 0x55cc91d51601 in clang::Parser::ParseStaticAssertDeclaration(clang::SourceLocation&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseDeclCXX.cpp:949:25
    #23 0x55cc91dc0cb8 in clang::Parser::ParseDeclaration(clang::DeclaratorContext, clang::SourceLocation&, clang::ParsedAttributes&, clang::SourceLocation*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseDecl.cpp:1794:18
    #24 0x55cc91bdb92e in clang::Parser::ParseExternalDeclaration(clang::ParsedAttributes&, clang::ParsingDeclSpec*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/Parser.cpp:932:14
    #25 0x55cc91bd3349 in clang::Parser::ParseTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, clang::Sema::ModuleImportState&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/Parser.cpp:727:12
    #26 0x55cc91bbb01e in clang::ParseAST(clang::Sema&, bool, bool) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseAST.cpp:162:20
    #27 0x55cc8e33d5d4 in clang::FrontendAction::Execute() /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Frontend/FrontendAction.cpp:1032:8
    #28 0x55cc8e16f390 in clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Frontend/CompilerInstance.cpp:1033:33
    #29 0x55cc8e5b6b76 in clang::ExecuteCompilerInvocation(clang::CompilerInstance*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:266:25
    #30 0x55cc84edab64 in cc1_main(llvm::ArrayRef<char const*>, char const*, void*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/tools/driver/cc1_main.cpp:248:15
    #31 0x55cc84ed3890 in ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/tools/driver/driver.cpp:317:12
    #32 0x55cc84ed1dfe in main /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/tools/driver/driver.cpp:388:12
    #33 0x7f14563da09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a) (BuildId: eb6a5dd378d22b1e695984462a799cd4c81cdc22)
SUMMARY: AddressSanitizer: 1632 byte(s) leaked in 3 allocation(s).
aaron.ballman added inline comments.Jun 1 2022, 1:07 PM
clang/test/SemaCXX/has_unique_object_reps_bitint.cpp
1

Huh, it looks to me like that leak may have been an existing issue that this test case uncovered (no part of that call stack would be impacted by the changes from this patch, as best I can tell).

@MitalAshok -- are you able to investigate the sanitizer behavior to validate my hunch?

Wow, thank you for the fantastic sleuthing! It seems that declaration is nine years old, so I'm surprised the leak is only being discovered now and as part of this particular test case.

I don't have a particularly easy way to test this locally at the moment -- do you know if switching VarTemplateSpecializationDecl::TemplateArgsInfo to be a ASTTemplateArgumentListInfo solves the issue for you?

Wow, thank you for the fantastic sleuthing! It seems that declaration is nine years old, so I'm surprised the leak is only being discovered now and as part of this particular test case.

I don't have a particularly easy way to test this locally at the moment -- do you know if switching VarTemplateSpecializationDecl::TemplateArgsInfo to be a ASTTemplateArgumentListInfo solves the issue for you?

Ah, ouch! This is definitely going to be the problem. TemplateArgumentList is generally just an 'observing' collection, stuff stored in the AST seems to need to use ASTTEmplateArgumentListInfo. The reason you might not notice it, is much of the time the former just references a bunch of template arguments stored elsewhere in the AST, so unless you hold it juuust right and find one that gets deleted before the rest of the AST, you won't have this problem. I'm not sure what is causing it in this test though.

Either way, I very much suggest we should make this change. Note there are a few places where this might be used that a conversion between the two will have to be made, but that is expected.