Variable locations for C++ structured bindings broken with optimisations #61981
Comments
|
@llvm/issue-subscribers-debuginfo |
OCHyams
added a commit
to OCHyams/llvm-project
that referenced
this issue
Oct 13, 2023
Addresses one part of llvm#61981: prior to this patch, upon splitting an alloca, SROA copied the dbg.declare onto the new allocas, updating the fragment info for each accordingly. This isn't enough; if there's an offset from the alloca in the DIExpression then it gets blindly copied to the new dbg.declare (i.e. the offset from the original alloca is copied over and applied to the new alloca). This patch fixes that by building on and using a utility funciton from Assignment Tracking called calculateFragmentIntersect to determine: a) The new fragment (existing code already did this), b) The new offset from the new alloca. Those parts are put together to create the new expression with a correct offset and fragment. Herein lies an key limitation of this approach too: calculateFragmentIntersect uses DIExression::extractIfOffset to grab an existing offset from the expression. extractIfOffset, and therefore calculateFragmentIntersect fails, if the expression contains anything other than an optional trivial offset and optional fragment. In that fail-state, debug-info for that variable fragment is dropped completely. That is to say, using this new approach, dbg.declares describing variable fragments that contain any non-offset non-fragment opcodes backed by allocas that are split are now dropped. If this is not an acceptable trade off, I suggest we simple undef/kill split alloca dbg.declares with offsets untill a more complete solution can be cooked up. That would probably involve interpreting the DIExpression and splicing the non-offset non-fragment part into the new expression. I'm not sure how difficult this is (it could be simple, but will probably be a little fiddly). Note: this fix doesn't address another similar issue. If two variables are stuffed into one alloca, then that alloca doesn't get split and mem2reg can promote it to a single (unsplit) value, the variable in the upper bits needs to have the offset in its DIExpression converted to a shift or a DW_OP_bit_piece (or otherwise describe that the variable starts at an offset into the value). I've called this out in the new test.
OCHyams
added a commit
to OCHyams/llvm-project
that referenced
this issue
Oct 13, 2023
Addresses one part of llvm#61981: prior to this patch, upon splitting an alloca, SROA copied the dbg.declare onto the new allocas, updating the fragment info for each accordingly. This isn't enough; if there's an offset from the alloca in the DIExpression then it gets blindly copied to the new dbg.declare (i.e. the offset from the original alloca is copied over and applied to the new alloca). This patch fixes that by building on and using a utility funciton from Assignment Tracking called calculateFragmentIntersect to determine: a) The new fragment (existing code already did this), b) The new offset from the new alloca. Those parts are put together to create the new expression with a correct offset and fragment. Herein lies an key limitation of this approach too: calculateFragmentIntersect uses DIExression::extractIfOffset to grab an existing offset from the expression. extractIfOffset, and therefore calculateFragmentIntersect fails, if the expression contains anything other than an optional trivial offset and optional fragment. In that fail-state, debug-info for that variable fragment is dropped completely. That is to say, using this new approach, dbg.declares describing variable fragments that contain any non-offset non-fragment opcodes backed by allocas that are split are now dropped. If this is not an acceptable trade off, I suggest we simple undef/kill split alloca dbg.declares with offsets untill a more complete solution can be cooked up. That would probably involve interpreting the DIExpression and splicing the non-offset non-fragment part into the new expression. I'm not sure how difficult this is (it could be simple, but will probably be a little fiddly). Note: this fix doesn't address another similar issue. If two variables are stuffed into one alloca, then that alloca doesn't get split and mem2reg can promote it to a single (unsplit) value, the variable in the upper bits needs to have the offset in its DIExpression converted to a shift or a DW_OP_bit_piece (or otherwise describe that the variable starts at an offset into the value). I've called this out in the new test.
OCHyams
added a commit
to OCHyams/llvm-project
that referenced
this issue
Jul 4, 2024
Fixes llvm#61981 by adjusting variable location offsets (in the DIExpression) when splittign allocas. AKA Fix debug locations for C++ structured bindings in SROA. NOTE: There's still a bug in mem2reg which generates incorrect locations in some situations: if the variable fragment has an offset into the new (split) alloca, mem2reg will fail to convert that into a bit shift (the location contains a garbage offset). That's not addressed here. insertNewDbgInst - Now takes the address-expression and FragmentInfo as serperate parameters because unlike dbg_declares dbg_assigns want those to go to different places. dbg_assign records put the variable fragment info in the value expression only (whereas dbg_declare has only one expression so puts it there - ideally this information wouldn't live in DIExpression, but that's another issue). MigrateOne - Modified to correctly compute the necessary offsets and fragment adjustments. The previous implementation produced bogus locations for variables with non-zero offsets. The changes replace most of the body of this lambda, so it might be easier to review in a split-diff view and focus on the change as a whole than to compare it to the old implementation. This uses calculateFragmentIntersect and extractLeadingOffset added in previous patches in this series, and createOrReplaceFragment described below. createOrReplaceFragment - Similar to DIExpression::createFragmentExpression except for 3 important distinctions: 1. The new fragment isn't relative to an existing fragment. 2. There are no checks on the the operation types because it is assumed the location this expression is computing is not implicit (i.e., it's always safe to create a fragment because arithmetic operations apply to the address computation, not to an implicit value computation). 3. Existing extract_bits are modified independetly of fragment changes using \p BitExtractOffset. A change to the fragment offset or size may affect a bit extract. But a bit extract offset can change independently of the fragment dimensions. Returns the new expression, or nullptr if one couldn't be created. Ideally this is only used to signal that a bit-extract has become zero-sized (and thus the new debug record has no size and can be dropped), however, it fails for other reasons too - see the FIXME below. FIXME: To keep this patch NFC the function bails in situations that DIExpression::createFragmentExpression fails in.E.g. when fragment and bit extract sizes differ. These limitations can be removed in the future.
OCHyams
added a commit
to OCHyams/llvm-project
that referenced
this issue
Jul 15, 2024
Fixes llvm#61981 by adjusting variable location offsets (in the DIExpression) when splittign allocas. AKA Fix debug locations for C++ structured bindings in SROA. NOTE: There's still a bug in mem2reg which generates incorrect locations in some situations: if the variable fragment has an offset into the new (split) alloca, mem2reg will fail to convert that into a bit shift (the location contains a garbage offset). That's not addressed here. insertNewDbgInst - Now takes the address-expression and FragmentInfo as serperate parameters because unlike dbg_declares dbg_assigns want those to go to different places. dbg_assign records put the variable fragment info in the value expression only (whereas dbg_declare has only one expression so puts it there - ideally this information wouldn't live in DIExpression, but that's another issue). MigrateOne - Modified to correctly compute the necessary offsets and fragment adjustments. The previous implementation produced bogus locations for variables with non-zero offsets. The changes replace most of the body of this lambda, so it might be easier to review in a split-diff view and focus on the change as a whole than to compare it to the old implementation. This uses calculateFragmentIntersect and extractLeadingOffset added in previous patches in this series, and createOrReplaceFragment described below. createOrReplaceFragment - Similar to DIExpression::createFragmentExpression except for 3 important distinctions: 1. The new fragment isn't relative to an existing fragment. 2. There are no checks on the the operation types because it is assumed the location this expression is computing is not implicit (i.e., it's always safe to create a fragment because arithmetic operations apply to the address computation, not to an implicit value computation). 3. Existing extract_bits are modified independetly of fragment changes using \p BitExtractOffset. A change to the fragment offset or size may affect a bit extract. But a bit extract offset can change independently of the fragment dimensions. Returns the new expression, or nullptr if one couldn't be created. Ideally this is only used to signal that a bit-extract has become zero-sized (and thus the new debug record has no size and can be dropped), however, it fails for other reasons too - see the FIXME below. FIXME: To keep this patch NFC the function bails in situations that DIExpression::createFragmentExpression fails in.E.g. when fragment and bit extract sizes differ. These limitations can be removed in the future.
OCHyams
added a commit
that referenced
this issue
Jul 18, 2024
Fixes issue #61981 by adjusting variable location offsets (in the DIExpression) when splitting allocas. Patch [4/4] to fix structured bindings in SROA. NOTE: There's still a bug in mem2reg which generates incorrect locations in some situations: if the variable fragment has an offset into the new (split) alloca, mem2reg will fail to convert that into a bit shift (the location contains a garbage offset). That's not addressed here. insertNewDbgInst - Now takes the address-expression and FragmentInfo as separate parameters because unlike dbg_declares dbg_assigns want those to go to different places. dbg_assign records put the variable fragment info in the value expression only (whereas dbg_declare has only one expression so puts it there - ideally this information wouldn't live in DIExpression, but that's another issue). MigrateOne - Modified to correctly compute the necessary offsets and fragment adjustments. The previous implementation produced bogus locations for variables with non-zero offsets. The changes replace most of the body of this lambda, so it might be easier to review in a split-diff view and focus on the change as a whole than to compare it to the old implementation. This uses calculateFragmentIntersect and extractLeadingOffset added in previous patches in this series, and createOrReplaceFragment described below. createOrReplaceFragment - Similar to DIExpression::createFragmentExpression except for 3 important distinctions: 1. The new fragment isn't relative to an existing fragment. 2. There are no checks on the the operation types because it is assumed the location this expression is computing is not implicit (i.e., it's always safe to create a fragment because arithmetic operations apply to the address computation, not to an implicit value computation). 3. Existing extract_bits are modified independetly of fragment changes using \p BitExtractOffset. A change to the fragment offset or size may affect a bit extract. But a bit extract offset can change independently of the fragment dimensions. Returns the new expression, or nullptr if one couldn't be created. Ideally this is only used to signal that a bit-extract has become zero-sized (and thus the new debug record has no size and can be dropped), however, it fails for other reasons too - see the FIXME below. FIXME: To keep the scope of this change focused on non-bitfield structured bindings the function bails in situations that DIExpression::createFragmentExpression fails. E.g. when fragment and bit extract sizes differ. These limitations can be removed in the future.
yuxuanchen1997
pushed a commit
that referenced
this issue
Jul 25, 2024
Summary: Fixes issue #61981 by adjusting variable location offsets (in the DIExpression) when splitting allocas. Patch [4/4] to fix structured bindings in SROA. NOTE: There's still a bug in mem2reg which generates incorrect locations in some situations: if the variable fragment has an offset into the new (split) alloca, mem2reg will fail to convert that into a bit shift (the location contains a garbage offset). That's not addressed here. insertNewDbgInst - Now takes the address-expression and FragmentInfo as separate parameters because unlike dbg_declares dbg_assigns want those to go to different places. dbg_assign records put the variable fragment info in the value expression only (whereas dbg_declare has only one expression so puts it there - ideally this information wouldn't live in DIExpression, but that's another issue). MigrateOne - Modified to correctly compute the necessary offsets and fragment adjustments. The previous implementation produced bogus locations for variables with non-zero offsets. The changes replace most of the body of this lambda, so it might be easier to review in a split-diff view and focus on the change as a whole than to compare it to the old implementation. This uses calculateFragmentIntersect and extractLeadingOffset added in previous patches in this series, and createOrReplaceFragment described below. createOrReplaceFragment - Similar to DIExpression::createFragmentExpression except for 3 important distinctions: 1. The new fragment isn't relative to an existing fragment. 2. There are no checks on the the operation types because it is assumed the location this expression is computing is not implicit (i.e., it's always safe to create a fragment because arithmetic operations apply to the address computation, not to an implicit value computation). 3. Existing extract_bits are modified independetly of fragment changes using \p BitExtractOffset. A change to the fragment offset or size may affect a bit extract. But a bit extract offset can change independently of the fragment dimensions. Returns the new expression, or nullptr if one couldn't be created. Ideally this is only used to signal that a bit-extract has become zero-sized (and thus the new debug record has no size and can be dropped), however, it fails for other reasons too - see the FIXME below. FIXME: To keep the scope of this change focused on non-bitfield structured bindings the function bails in situations that DIExpression::createFragmentExpression fails. E.g. when fragment and bit extract sizes differ. These limitations can be removed in the future. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250875
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Clang/LLVM version: 1a0653a (5th April 2023).
Variable locations for structured binding are incorrect for promoted variables
after SROA.
Consider this test case:
Build:
$ clang -g test.cpp -O2 -Xclang -fexperimental-assignment-tracking=disabled -o testDebug:
Variable
xis in$raxwith value 5 (correct). Variableyis reportedly at address$rax+4which is clearly incorrect. It should be in$rcxwith value 10 (see below).Objdump:
It's SROA that is introducing this error. See below - the
DW_OP_plus_uconst, 4offset iny's (!31)dbg.declareis incorrectly carried over to itsdbg.value, which also incorrectly uses the value%.sroa.0.0.extract.trunc(which is the value of the lower 32 bits of the original alloca - i.e. variablex).The text was updated successfully, but these errors were encountered: