This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by shchenz on Apr 6 2021, 9:38 PM.

Details

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.

Diff Detail

Event Timeline

shchenz created this revision.Apr 6 2021, 9:38 PM
shchenz requested review of this revision.Apr 6 2021, 9:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2021, 9:38 PM
aprantl added inline comments.Apr 8 2021, 9:52 AM
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 updated this revision to Diff 336290.Apr 8 2021, 6:44 PM
shchenz marked 2 inline comments as done.

1: check the DWARF info instead of the crash

Thanks for your review @aprantl . Updated accordingly.

llvm/test/CodeGen/Generic/pr49872.ll
1

Good idea. I changed the checks for DWARF info.

aprantl accepted this revision.Apr 9 2021, 12:32 PM
aprantl added inline comments.
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.

This revision is now accepted and ready to land.Apr 9 2021, 12:32 PM
shchenz added inline comments.Apr 11 2021, 8:08 PM
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 removed a subscriber: LiuChen3.
LiuChen3 added inline comments.Jul 28 2021, 12:53 AM
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 added inline comments.Jul 28 2021, 2:12 AM
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 added inline comments.
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 added inline comments.Jul 30 2021, 2:16 PM
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 added inline comments.Jul 31 2021, 8:12 PM
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 added inline comments.Jul 31 2021, 8:23 PM
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 added inline comments.Aug 1 2021, 5:46 PM
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 added inline comments.Aug 1 2021, 5:55 PM
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.