[debug-info] make fortran CHARACTER(1) type as valid unsigned type

[shchenz]

Summary

This resolves https://bugs.llvm.org/show_bug.cgi?id=49872

SROA will create llvm.dbg.value to assign an unsigned integer to a variable with the type CHARACTER(1). For now, this dbg value will cause llc crash in AsmPrinter pass.

[aprantl]

llvm/test/CodeGen/Generic/pr49872.ll

1

wouldn't you want to pipe this through dwarfdump or emit assembler output?

4

integer

7

Could we check anything slightly more substantial here?

[shchenz]

1: check the DWARF info instead of the crash

[shchenz]

Thanks for your review @aprantl . Updated accordingly.

llvm/test/CodeGen/Generic/pr49872.ll

1

Good idea. I changed the checks for DWARF info.

[aprantl]

llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp

180

It would be cleaner to reject this in Verifier and make it part of the general expectations for LLVM IR metadata.

[shchenz]

llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp

180

Yes, agree. I am thinking we can add verifications for a DbgValueInst about the const value and the variable type. With these new verifications, we can expose this issue earlier. I will do this later in another patch when I get some time. Thanks for your review. @aprantl

[LiuChen3]

llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp

180

Hi, @shchenz . This assertion make our testcase fail on X86 target. I am not familiar with this part so I have a question here: why the type here must be unsigned? Should this be something like "return Ty->getSizeInBits() == 8; " ?
Thanks.

[shchenz]

llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp

180

I added this assertion only for i8 is because SROA will convert Fortran CHARACTER(1) type (DIStringType) to i8. Do you also get some DIStringType variables being converted to a scalar type, like i8, i16, i32? If so, I think you need to first check if the above conversion is valid or not. if it is valid, then you can change the assertion here.

[LiuChen3]

llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp

180

Our test is too big and I didn't find the root cause now.
I am just confused about the assertion here. If the DIStringType must be i8, I think you should insert assertion after the convert(Maybe in SROA), instead of inserting assertion here. Here should just return false if the type isn't i8.

[aprantl]

llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp

180

We have many tools like delta (https://web.archive.org/web/20200701152100/http://delta.tigris.org/) bugpoint, and llvm-reduce available which can automate the process of reducing a testcase.

[LiuChen3]

llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp

180

The spec is too big and this fail happened on link time. With '-g' make it slower.
I still wonder why you think the DIStringType here must be unsigned char? No i16, i32 or some types else?

[shchenz]

llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp

180

[1xi8] should be ok to convert to i8 as the data layout would be no change. Imaging char -> int in .c. But [2xi8](CHARACTER(2)) should be non-ok to convert to i16 as the data layout is not the same, we can not convert char[2] to int?

[LiuChen3]

llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp

180

You should insert the assertion in SROA if you thought the [1xi8] must do this convert. No assumptions should be made about type of DIStringType here.
Suppose this function is like this: bool ischar(Type ty)
Here 'ty' can be 'char', 'int', 'long long' or any types. Just return false if the type isn't 'char' instead of crashing.

[LiuChen3]

llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp

180

I understand what you mean a little bit. The DIStringType can only be i8 here. But I still feel that the assertion here does not match the meaning of the function. I will continue to try to find out if some of our passes are wrong as you said. Thanks.

llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp

uint64_t DebugHandlerBase::getBaseTypeSize(const DIType *Ty) {
   if (BaseType->getTag() == dwarf::DW_TAG_reference_type ||
       BaseType->getTag() == dwarf::DW_TAG_rvalue_reference_type)
     return Ty->getSizeInBits();
 
   return getBaseTypeSize(BaseType);
 }
 
 bool DebugHandlerBase::isUnsignedDIType(const DIType *Ty) {
+  // SROA may generate dbg value intrinsics to assign an unsigned value to a
+  // Fortran CHARACTER(1) type variables. Make them as unsigned.
+  if (isa<DIStringType>(Ty)) {
+    assert((Ty->getSizeInBits()) == 8 && "Not a valid unsigned type!");
+    return true;
+  }
   if (auto *CTy = dyn_cast<DICompositeType>(Ty)) {
     // FIXME: Enums without a fixed underlying type have unknown signedness
     // here, leading to incorrectly emitted constants.
     if (CTy->getTag() == dwarf::DW_TAG_enumeration_type)
       return false;
 
     // (Pieces of) aggregate types that get hacked apart by SROA may be
     // represented by a constant. Encode them as unsigned bytes.

llvm/test/CodeGen/Generic/pr49872.ll

+; RUN: llc < %s | FileCheck %s
+
+; PR49872
+; Check a interger constant for Fortran CHARACTER(1) will not cause a crash in
+; DebugHandlerBase::isUnsignedDIType when calling DwarfUnit::addConstantValue
+
+; CHECK-LABEL: s1_after_sroa
+
+source_filename = "1.ll"
+
+define dso_local void @s1_after_sroa([1 x i8] %arg) local_unnamed_addr #0 !dbg !5 {            
+s1_entry:                                                                           
+  call void @llvm.dbg.value(metadata i8 122, metadata !10, metadata !DIExpression()), !dbg !11
+  ret void, !dbg !12                                                                
+}                                                                                   
+
+declare void @llvm.dbg.value(metadata, metadata, metadata) #5
+
+!llvm.module.flags = !{!0, !1}
+!llvm.dbg.cu = !{!2}
+
+!0 = !{i32 2, !"Dwarf Version", i32 3}
+!1 = !{i32 2, !"Debug Info Version", i32 3}
+!2 = distinct !DICompileUnit(language: DW_LANG_Fortran95, file: !3, isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !4, splitDebugInlining: false)
+!3 = !DIFile(filename: "1.f", directory: ".")
+!4 = !{}
+!5 = distinct !DISubprogram(name: "s1", linkageName: "s1", scope: !3, file: !3, line: 1, type: !6, scopeLine: 1, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !2, retainedNodes: !4)
+!6 = !DISubroutineType(types: !7)
+!7 = !{!8, !9}
+!8 = !DIBasicType(tag: DW_TAG_unspecified_type, name: "void")
+!9 = !DIStringType(name: "char string", size: 8)
+!10 = !DILocalVariable(name: "arg", arg: 1, scope: !5, file: !3, type: !9)
+!11 = !DILocation(line: 0, scope: !5)
+!12 = !DILocation(line: 4, scope: !5)
+!13 = !DILocation(line: 5, scope: !5)
+!14 = distinct !DISubprogram(name: "p", linkageName: "main", scope: !3, file: !3, line: 7, type: !15, scopeLine: 7, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !2, retainedNodes: !4)
+!15 = !DISubroutineType(types: !16)
+!16 = !{!17}
+!17 = !DIBasicType(name: "INTEGER", size: 32, encoding: DW_ATE_signed)
+!18 = !DILocation(line: 8, scope: !14)