This is an archive of the discontinued LLVM Phabricator instance.

[Verifier] Sanity check alloca size against DILocalVariable fragment size
ClosedPublic

Authored by nikic on Aug 24 2023, 7:58 AM.

Diff Detail

Event Timeline

nikic created this revision.Aug 24 2023, 7:58 AM
nikic requested review of this revision.Aug 24 2023, 7:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2023, 7:58 AM
nikic added inline comments.Aug 24 2023, 8:00 AM
llvm/lib/IR/Verifier.cpp
6295

This is my attempt to only handle empty DIExpression or DW_OP_LLVM_fragment, but not things like DW_OP_deref. Is there some cleaner way to check this?

nikic updated this revision to Diff 553165.Aug 24 2023, 9:31 AM
nikic edited the summary of this revision. (Show Details)

Update one more test.

aprantl requested changes to this revision.Aug 24 2023, 11:07 AM
aprantl added a subscriber: aprantl.
aprantl added inline comments.
llvm/lib/IR/Verifier.cpp
6295

Yeah, you need to check that the expression directly points into the alloca and isn't the result computation that uses the contents of the alloca as an input.

This revision now requires changes to proceed.Aug 24 2023, 11:07 AM
aprantl accepted this revision.Aug 24 2023, 11:09 AM
aprantl added inline comments.
llvm/lib/IR/Verifier.cpp
6295

!isComplex() might do what you want.

This revision is now accepted and ready to land.Aug 24 2023, 11:09 AM
nikic updated this revision to Diff 553414.Aug 25 2023, 2:18 AM
nikic edited the summary of this revision. (Show Details)

Use isComplex(), update failing tests. I've split X86/stack-frame-layout-remarks.ll into two files for 64-bit and 32-bit.

nikic updated this revision to Diff 553416.Aug 25 2023, 2:19 AM

Actually do the isComplex() change...

nikic added inline comments.Aug 25 2023, 2:20 AM
llvm/lib/IR/Verifier.cpp
6295

Thanks, isComplex() was indeed what I was looking for.

aprantl added inline comments.Aug 25 2023, 10:00 AM
llvm/lib/IR/Verifier.cpp
6295

We should come up with a better name for isComplex().
Anyway, this LGTM now!

nikic added a comment.Aug 28 2023, 2:09 AM

Looks like this found a legitimate issue in Clang debuginfo for trivial_abi with non-trivial dtor: https://clang.godbolt.org/z/sWsEMvWas The dbg.declare points to an i1 %nrvo variable, rather than %retval.

nikic reopened this revision.Aug 28 2023, 2:57 AM
This revision is now accepted and ready to land.Aug 28 2023, 2:57 AM
nikic updated this revision to Diff 553880.Aug 28 2023, 3:07 AM

I've submitted D158972 to fix the clang bug.

An additional problem that turned up is that a number of tests that don't specify a target have 64-bit specific debuginfo and thus fail when tested against 32-bit targets. I've fixed these either by tweaking things to be pointer size independent, or by testing these using the x86_64 target. (The Generic/dwarf-public-names.ll test got dropped entirely, because X86/dwarf-public-names.ll already exists and is more extensive to boot.)

It looks like we're hitting this new assertion in rust built with llvm @ HEAD in a few places (after @durin42's https://github.com/llvm/llvm-project/commit/1f33911f50294c07f672a49e311776693823d0bc, which fixed some segfaults):
https://buildkite.com/llvm-project/rust-llvm-integrate-prototype/builds/22373#018aa862-a088-4d6e-a9a7-967348e12f4b/824-825

@nikic could you please take a look? Also would it be OK if you reverted this temporarily until we fix the rust side (internally we're building rust and clang with llvm close to head and this prevents us from building rust)?

nikic added a comment.Sep 18 2023, 8:28 AM

It looks like we're hitting this new assertion in rust built with llvm @ HEAD in a few places (after @durin42's https://github.com/llvm/llvm-project/commit/1f33911f50294c07f672a49e311776693823d0bc, which fixed some segfaults):
https://buildkite.com/llvm-project/rust-llvm-integrate-prototype/builds/22373#018aa862-a088-4d6e-a9a7-967348e12f4b/824-825

@nikic could you please take a look? Also would it be OK if you reverted this temporarily until we fix the rust side (internally we're building rust and clang with llvm close to head and this prevents us from building rust)?

Okay, I've reverted this for now. I've posted a small reproducer for the rustc bug in https://rust-lang.zulipchat.com/#narrow/stream/187780-t-compiler.2Fwg-llvm/topic/LLVM.20HEAD.20llvm.2Edbg.2Edeclare.2Falloca.20size.20mismatch.