Page MenuHomePhabricator

[DebugInfo] Do not emit entry values for composite locations
AcceptedPublic

Authored by dstenb on Feb 27 2020, 9:06 AM.

Details

Summary

This is a fix for PR45009.

When working on D67492 I made DwarfExpression emit a single
DW_OP_entry_value operation covering the whole composite location
description that is produced if a register does not have a valid DWARF
number, and is instead composed of multiple register pieces. Looking
closer at the standard, I realized that that is not valid DWARF. A
DW_OP_entry_value operation's block can only be a DWARF expression or a
register location description, so it is not valid for it to hold a
composite location description like that.

See DWARFv5 sec. 2.5.1.7:

"The DW_OP_entry_value operation pushes the value that the described
location held upon entering the current subprogram. It has two
operands: an unsigned LEB128 length, followed by a block containing a
DWARF expression or a register location description (see Section
2.6.1.1.3 on page 39)."

Perhaps we instead want to emit a entry value operation per each
DW_OP_reg* operation, e.g.:

  • DW_OP_entry_value(DW_OP_regx sub_reg0), DW_OP_stack_value, DW_OP_piece 8,
  • DW_OP_entry_value(DW_OP_regx sub_reg1), DW_OP_stack_value, DW_OP_piece 8, [...]

The question then becomes how the call site should look; should a
composite location description be emitted there, and we then leave it up
to the debugger to match those two composite location descriptions?
Another alternative could be to emit a call site parameter entry for
each sub-register, but firstly I'm unsure if that is even valid DWARF,
and secondly it seems like that would complicate the collection of call
site values quite a bit. As far as I can tell GCC does not emit any
entry values / call sites in these cases, so we do not have something to
compare with, but the former seems like the more reasonable approach.

Currently when trying to emit a call site entry for a parameter composed
of multiple DWARF registers a (DwarfRegs.size() == 1) assert is
triggered in addMachineRegExpression(). Until the call site
representation is figured out, and until there is use for these entry
values in practice, this commit simply stops the invalid DWARF from
being emitted.

Diff Detail

Unit TestsFailed

TimeTest
1,310 msMLIR.mlir-cpu-runner::bare_ptr_call_conv.mlir
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/mlir-opt /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/mlir/test/mlir-cpu-runner/bare_ptr_call_conv.mlir -convert-loop-to-std -convert-std-to-llvm='use-bare-ptr-memref-call-conv=1' | mlir-cpu-runner -shared-libs=/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/libmlir_runner_utils.so -entry-point-result=void | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/mlir/test/mlir-cpu-runner/bare_ptr_call_conv.mlir
1,340 msMLIR.mlir-cpu-runner::unranked_memref.mlir
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/mlir-opt /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/mlir/test/mlir-cpu-runner/unranked_memref.mlir -convert-linalg-to-loops -convert-linalg-to-llvm -convert-std-to-llvm | mlir-cpu-runner -e main -entry-point-result=void -shared-libs=/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/libmlir_runner_utils.so,/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/libcblas.so,/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/libcblas_interface.so | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/mlir/test/mlir-cpu-runner/unranked_memref.mlir
1,230 msMLIR.mlir-cpu-runner::utils.mlir
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/mlir-opt /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/mlir/test/mlir-cpu-runner/utils.mlir -convert-linalg-to-loops -convert-linalg-to-llvm -convert-std-to-llvm | mlir-cpu-runner -e print_0d -entry-point-result=void -shared-libs=/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/libmlir_runner_utils.so | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/mlir/test/mlir-cpu-runner/utils.mlir --check-prefix=PRINT-0D
2,020 mslibc++.std/thread/thread_mutex/thread_mutex_requirements/thread_timedmutex_requirements/thread_timedmutex_recursive::try_lock_for.pass.cpp
Compiled With: '/usr/bin/clang++ -o /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/projects/libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.timedmutex.requirements/thread.timedmutex.recursive/Output/try_lock_for.pass.cpp.o -x c++ /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.timedmutex.requirements/thread.timedmutex.recursive/try_lock_for.pass.cpp -c -v -ftemplate-depth=270 -Werror=thread-safety -std=c++2a -include /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/test/support/nasty_macros.h -nostdinc++ -I/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/include -I/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/test/support -DLIBCXX_FILESYSTEM_STATIC_TEST_ROOT="/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/test/std/input.output/filesystems/Inputs/static_test_env" -DLIBCXX_FILESYSTEM_DYNAMIC_TEST_ROOT="/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/projects/libcxx/test/filesystem/Output/dynamic_env" -DLIBCXX_FILESYSTEM_DYNAMIC_TEST_HELPER="/usr/bin/python /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/test/support/filesystem_dynamic_test_helper.py" -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -Wall -Wextra -Werror -Wuser-defined-warnings -Wshadow -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -c && /usr/bin/clang++ -o /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/projects/libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.timedmutex.requirements/thread.timedmutex.recursive/Output/try_lock_for.pass.cpp.exe…

Event Timeline

dstenb created this revision.Feb 27 2020, 9:06 AM
vsk accepted this revision.EditedFeb 27 2020, 5:15 PM

Fixing the assertion failure in this way sgtm. Re:

DW_OP_entry_value(DW_OP_regx sub_reg0), DW_OP_stack_value, DW_OP_piece 8, DW_OP_entry_value(DW_OP_regx sub_reg1), DW_OP_stack_value, DW_OP_piece 8, ...

It sounds like you expect debuggers to have some difficulty handling this expression. How come? I didn't understand this part.

My lack of familiarity with DWARF is showing now, but would DW_OP_entry_value(DW_OP_regx sub_reg0, DW_OP_piece 8, DW_OP_regx sub_reg1, DW_OP_piece 8) be valid (sorry if I've left out any necessary OP_stack_values)?

The question then becomes how the call site should look; should a
composite location description be emitted there, and we then leave it up
to the debugger to match those two composite location descriptions?

This seems reasonable to me, but I feel like I'm missing something here..

Another alternative could be to emit a call site parameter entry for
each sub-register,

I believe this would complicate the debugger support significantly, hopefully there's a simpler way to do this. Right now lldb just does a 'memcmp' to figure out whether to use an entry value. If there were a separate call site parameter entry for each subregister in the caller that forms a register used in the callee, that would have to change into a more complicated merging operation.

This revision is now accepted and ready to land.Feb 27 2020, 5:15 PM
djtodoro accepted this revision.Feb 28 2020, 12:45 AM
In D75270#1897102, @vsk wrote:

Another alternative could be to emit a call site parameter entry for
each sub-register,

I believe this would complicate the debugger support significantly, hopefully there's a simpler way to do this.

+1

In D75270, @dstenb wrote:

As far as I can tell GCC does not emit any
entry values / call sites in these cases, so we do not have something to
compare with, but the former seems like the more reasonable approach.

I think as well. What about GDB side? Did GDB expect something like this? I though just running GDB over such case could be interesting to see if it accepts such scenario or not (I should find some time to play with that for sure).

As far as I can tell GCC does not emit any
entry values / call sites in these cases, so we do not have something to
compare with, but the former seems like the more reasonable approach.

I think as well. What about GDB side? Did GDB expect something like this? I though just running GDB over such case could be interesting to see if it accepts such scenario or not (I should find some time to play with that for sure).

GDB currently imposes the following restrictions for call site locations:

Only single DW_OP_reg or DW_OP_fbreg is supported for DW_FORM_block* DW_AT_location is supported for DW_TAG_call_site child DIE

and the following for entry value operations:

DWARF-2 expression error: DW_OP_entry_value is supported only for single DW_OP_reg* or for DW_OP_breg*(0)+DW_OP_deref*

As far as I can tell GCC does not emit any
entry values / call sites in these cases, so we do not have something to
compare with, but the former seems like the more reasonable approach.

I think as well. What about GDB side? Did GDB expect something like this? I though just running GDB over such case could be interesting to see if it accepts such scenario or not (I should find some time to play with that for sure).

GDB currently imposes the following restrictions for call site locations:

Only single DW_OP_reg or DW_OP_fbreg is supported for DW_FORM_block* DW_AT_location is supported for DW_TAG_call_site child DIE

and the following for entry value operations:

DWARF-2 expression error: DW_OP_entry_value is supported only for single DW_OP_reg* or for DW_OP_breg*(0)+DW_OP_deref*

Thanks for this! I've expected that... I've confirmed this output with an older version of GDB 7.1* as well. We will need to discuss about adding such support to debuggers.

dstenb added a comment.Mar 4 2020, 4:47 AM
In D75270#1897102, @vsk wrote:

Fixing the assertion failure in this way sgtm. Re:

Sorry, coming back to this now after the similar discussion in D75326.

I just want to clarify that this patch does not fix any assertion failures. The entry value operation is emitted without crashing; it's just that I think that it's invalid DWARF. The assertion I mentioned in the description was for the DW_AT_location at the call site parameter entry (but see my correction about that at the end of this comment).

DW_OP_entry_value(DW_OP_regx sub_reg0), DW_OP_stack_value, DW_OP_piece 8, DW_OP_entry_value(DW_OP_regx sub_reg1), DW_OP_stack_value, DW_OP_piece 8, ...

It sounds like you expect debuggers to have some difficulty handling this expression. How come? I didn't understand this part.

I don't know how much about how the call site and entry value matching works and is implemented in the debuggers, but I thought there could be some difficulties to understand that the each sub-register's entry value at the callee's location:

DW_OP_entry_value(DW_OP_regx sub_reg0), DW_OP_stack_value, DW_OP_piece 8, DW_OP_entry_value(DW_OP_regx sub_reg1), DW_OP_stack_value, DW_OP_piece 8, ...

should be matched to the corresponding piece of the DW_AT_location for the call site's single parameter entry:

DW_OP_regx sub_reg0, DW_OP_piece 8, DW_OP_regx sub_reg1, DW_OP_stack_value, DW_OP_piece 8, ...

Then, if we e.g. have a DW_OP_constu as the DW_AT_call_value for that call site parameter entry, I assume that the debugger has to split up that value into different parts according to the DW_AT_location composition, and push those to the DWARF stacks for the entry values in the different pieces?

Again, as I don't know much about the debuggers' implementations, maybe nothing of that is an issue really?

My lack of familiarity with DWARF is showing now, but would DW_OP_entry_value(DW_OP_regx sub_reg0, DW_OP_piece 8, DW_OP_regx sub_reg1, DW_OP_piece 8) be valid (sorry if I've left out any necessary OP_stack_values)?

That is what we currently emit (with a stack value after the DW_OP_entry_value). I think that's invalid DWARF since the standard specifies that the DW_OP_entry_value operation's block is "containing a DWARF expression or a register location description", so I don't think it's valid to emit a composite location description like that one.

And sorry, I noticed something incorrect in my patch description:

In D75270, @dstenb wrote:

Currently when trying to emit a call site entry for a parameter composed
of multiple DWARF registers a (DwarfRegs.size() == 1) assert is
triggered in addMachineRegExpression(). Until the call site
representation is figured out, and until there is use for these entry
values in practice, this commit simply stops the invalid DWARF from
being emitted.

The call site entry's DW_AT_location is emitted fine for composite locations, so what I'm describing there does not happen for those cases. However, if you were to describe the call site value using a preserved register, which is composed of multiple DWARF register pieces, you'll currently hit that assertion (code here: [0]) since isParameterValue() is true.

[0] https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp#L279

Hi @dstenb,

What is the status with this?

Hi @dstenb,

What is the status with this?

Sorry for leaving this!

This related a bit to D75326, and there I sent a mail to the Dwarf-Discuss mailing thread (http://lists.dwarfstd.org/pipermail/dwarf-discuss-dwarfstd.org/2020-March/004610.html) about whether or not composite locations are allowed in DWARF Expressions.

@vsk, do you think it would be okay for you if we land this patch based on the similar reasoning as in you wrote in D75326? I mean this:

Reading through the discussion, I do see some amount of consensus about the first sentence of section 2.5 being misleading (or at least confusing) -- this is the bit I cited in argument for allowing composite locations wherever a value is expected. I think we'd need a strong reason to disregard that consensus.