This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Add DW_ATE_complex_float case to assert in isUnsignedDIType
ClosedPublic

Authored by Orlando on May 31 2023, 4:35 AM.

Details

Summary

Without this patch a DW_ATE_complex_float encoding trips an assertion in DebugHandlerBase::isUnsignedDIType. The assertion message "Unsupported encoding" possibly indicates that either the encoding either truly isn't supported or hasn't yet had support added.

By adding a case to the assert for DW_ATE_complex_float it becomes supported, behaving in the same way as the already supported DW_ATE_float type (return false), which I think makes sense.

That resolves this issue mentioned in D146987.

Note: For the reported reproducer:

#include <complex.h>
int main() {
  long double complex r1;
}

The assertion isn't tripped without assignment tracking because instcombine deletes everything, including the dbg.declare, without recovering any location information. Whereas with assignment tracking we track a zeroing memset that is emitted by clang.

Diff Detail

Event Timeline

Orlando created this revision.May 31 2023, 4:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2023, 4:35 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
Orlando requested review of this revision.May 31 2023, 4:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2023, 4:35 AM
MaskRay added inline comments.
llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp
226

Whether are the other Encoding == xxx tested? Is it possible to group the DW_ATE_complex_float test with those test files?

llvm/test/DebugInfo/X86/DW_ATE_complex_float.ll
1

-filetype=asm is the default action and usually omitted.

Orlando updated this revision to Diff 527334.Jun 1 2023, 2:34 AM
Orlando marked 2 inline comments as done.

+ address review comments

Thanks for the review

llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp
226

Omitting some types from the assertion and running the tests, among other failures I found float_const.ll, dbg-i128-const.ll and dbg-const-int.ll in llvm/test/DebugInfo/X86 (same directory as the new test) which look to be testing this behaviour. Not conclusive but it looks like there's a precedent for using this directory.

probinson added inline comments.
llvm/test/DebugInfo/X86/DW_ATE_complex_float.ll
12

This shows we have a const_value, but not what it's part of. More context for the check would be good.

Orlando updated this revision to Diff 529844.Jun 9 2023, 12:37 AM
Orlando marked an inline comment as done.

+ Added more CHECK lines to the test

probinson accepted this revision.Jun 9 2023, 6:40 AM

LGTM

llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp
226

I suspect we could improve testing efficiency by combining some of these into a single test file, but as they are already split across files I don't think it's appropriate to insist that you do anything different here.

This revision is now accepted and ready to land.Jun 9 2023, 6:40 AM
MaskRay added inline comments.Jun 9 2023, 12:55 PM
llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp
226

I agree. It'd be worth thinking about grouping tests if @Orlando or others have extra energy on the work:)