This patch extends DIModule Debug metadata in LLVM to support Fortran modules. DIModule is extended to contain File and Line fields, these fields will be used by Flang FE to create debug information necessary for representing Fortran modules at IR level.
Furthermore DW_TAG_module is also extended to contain these fields. If these fields are missing, debuggers like GDB
won't be able to show Fortran modules information correctly.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Generally, this sounds reasonable, thanks!
Is the line and file is necessary because in Fortran you may have more than one module declaration per file? Out of curiosity, what does the debugger use the line/file info for?
In addition to the inline comments:
Please make sure to provide a bitcode/assembler roundtrip test, maybe in test/Assembler/dimodule.ll
I'm unsure that the metadataloader implementation is correct. Can you also add a bitcode upgrade test to test/Bitcode/ that shows that the older, shorter format is handled correctly?
llvm/include/llvm/IR/DIBuilder.h | ||
---|---|---|
745 | /// \param File Source file of the module declaration. Used for Fortran modules. | |
748 | same here | |
752 | This makes it quite opaque what the actual default value is. | |
llvm/include/llvm/IR/DebugInfoMetadata.h | ||
2081 | /// Represents a module in the programming language, for example, a Clang module, or a Fortran module. | |
2139 | The parent class DIScope already provides a getFile() method. Are we storing the file operand twice? | |
llvm/lib/Bitcode/Reader/MetadataLoader.cpp | ||
1430 | Will this crash when we get an input with only 6 records? Is there a bitcode upgrade test that shows that the older, shorter format is handled gracefully? | |
llvm/lib/IR/LLVMContextImpl.h | ||
849 | How likely is it going to be that you have two fortran modules with the same name but a different, file or lineno? Is it necessary for performance to hash the file/line? |
Thanks for review @aprantl .
Please make sure to provide a bitcode/assembler roundtrip test, maybe in test/Assembler/dimodule.ll
I worked on this too, Unfortunately due to messy workspace, I missed out to add this. Meanwhile I have this locally. I guess this test case addresses another concern of yours:
Will this crash when we get an input with only 6 records?
Is the line and file is necessary because in Fortran you may have more than one module declaration per file? Out of curiosity, what does the debugger use the line/file info for?
Yes this is necessary, GDB uses these line/file to display fortran modules info:
(gdb) info modules File test.f90: 18: mod2
llvm/include/llvm/IR/DebugInfoMetadata.h | ||
---|---|---|
2139 | Didn't get you completely here, I think that's needed, if you're asking. |
Addressed @aprantl comments, Thanks for this!
- Added bitcode round trip test cases to ensure backward compatibility and current version too.
- Added missing test case Assembler/dimodule.ll.
- Addressed other comments.
llvm/lib/IR/LLVMContextImpl.h | ||
---|---|---|
849 | Thanks for pointing this out! This seems like an unlikely situation, therefore removed it. |
I assume there are corresponding changes to flang. Can you provide a link to them? Thanks.
llvm/lib/Bitcode/Reader/MetadataLoader.cpp | ||
---|---|---|
1421 | It looks like 2 new fields are being added, but only 1 here. |
I assume there are corresponding changes to flang. Can you provide a link to them? Thanks.
We're yet raise a PR on flang about the corresponding changes. Planning to do it soon, once this is in. BTW we tested flang internally after applying this patch, GDB is able show the modules info.
llvm/lib/Bitcode/Reader/MetadataLoader.cpp | ||
---|---|---|
1421 | Sorry! didn't get you exactly. Two fields 6, 7 are added to capture file, line info. |
llvm/include/llvm/IR/DebugInfoMetadata.h | ||
---|---|---|
2139 |
We'll have to figure that out first. |
Addressed @aprantl review comments, Thanks for this!
- Made DIFile as first metadata operand and removed getFile from DIModule, now it's accessible via DIScope.
llvm/include/llvm/IR/DIBuilder.h | ||
---|---|---|
746 | Nit: /// Fortran modules. | |
748 | indentation again | |
751 | = nullptr is also more obvious | |
llvm/include/llvm/IR/DebugInfoMetadata.h | ||
2081 | double space after programming | |
llvm/lib/AsmParser/LLParser.cpp | ||
4831 | missing space after , | |
llvm/lib/Bitcode/Reader/MetadataLoader.cpp | ||
1430 | This still looks wrong! This will access beyond Record.size() if Record.size() == 6. | |
llvm/lib/IR/LLVMContextImpl.h | ||
843–844 | Here you do need to compare all fields! It's just not necessary to hash them. | |
llvm/unittests/IR/MetadataTest.cpp | ||
2062 | let's use a real, nonzero number here | |
2085 | You need to add two extra tests here. One where only File is different, one where only Line is different. (We can skip the hashing for performance, but they still must *compare* differently) |
Addressed @aprantl comments. Thanks for this!
- Added 2 unit test cases in MetadataTest.cpp
- Corrected Metadata loading based on record size.
- Corrected and added compare fields in LLVMContextImpl.
- Corrected all Nits.
llvm/lib/Bitcode/Reader/MetadataLoader.cpp | ||
---|---|---|
1430 | Is this Okay ? if record size is 6 then let the other records null and only access them otherwise. | |
llvm/lib/IR/LLVMContextImpl.h | ||
843–844 | Thanks for correcting here. I've added the both fields for comparison. | |
llvm/unittests/IR/MetadataTest.cpp | ||
2085 | Done! Thanks! |
llvm/lib/Bitcode/Reader/MetadataLoader.cpp | ||
---|---|---|
1431 | You could still attack the loader with malformed input here: if (Record.size() < 5 || Record.size() > 8) means we accept a size of 6, 7, or 8. That means that Record.size() == 6 ? 0 : Record[7])), will access Record[7] if the size is 7. I would use Record.size() <= 6 ? nullptr : ... and Record.size() <= 7 ? 0 : ..., respectively. |
llvm/lib/Bitcode/Reader/MetadataLoader.cpp | ||
---|---|---|
1431 | Thanks! for the suggestion. Code seems more robust and consistent now. |
llvm/include/llvm/IR/DIBuilder.h | ||
---|---|---|
747 | /// \param LineNo Source line number of the module declaration. | |
llvm/lib/Bitcode/Reader/MetadataLoader.cpp | ||
1431 | There is one last thing that I still don't understand: The File is metadata operand 0 (= bitcode Record[1]), and the BitcodeWriter writes out the operands in order (as it should). How does this pass the bitcode upgrade test? Did you remember to create the .bc file with the unmodified llvm-as? Shouldn't the have to reader take into account that we inserted a field in the beginning and thus shift all records by one? Maybe I'm missing something, but I would expect that we need something like: unsigned Offset = Record.size() >= 7 ? 1 : 2; Record[1+Offset], Record[2+Offset], ... here |
llvm/lib/IR/DebugInfoMetadata.cpp | ||
---|---|---|
744 | Is this clang-formated ? |
llvm/lib/IR/DebugInfoMetadata.cpp | ||
---|---|---|
744 | Yes! |
Addressed @aprantl comments, Thanks for this!
- Now Bitcode backward/forward compatibility is maintained.
llvm/lib/Bitcode/Reader/MetadataLoader.cpp | ||
---|---|---|
1431 | Thanks for pointing this out! In this revision, this file is generated using unmodified(i.e before this patch) llvm-as, and llvm-dis is able to correctly disassemble it. |
I think this should work. Please watch the bots carefully, particularly the stage2 LTO ones after landing this.
llvm/lib/IR/DebugInfoMetadata.cpp | ||
---|---|---|
744 | Yeah, that's something clang-format will actually do. |
This breaks tests on Windows: http://45.33.8.238/win/15149/step_11.txt
Please take a look and revert if it takes a while to investigate.
I've pushed a fix for the test here: https://github.com/llvm/llvm-project/commit/a520c89a4768f1e2ad7012eb70ff05357fc5e985
Thanks for fixing this! Unfortunately didn't received for this failure. I didn't intend to run this on Windows that's why inserted the triple, Seem like %llc_dwarf automatically picks-up/append the systems default triple hence the overall test was failing.
@aprantl , Do we also need to upgrade release notes for this change? I mean DIModule has now 2 more fields compared to previous one.
In the past I have never updated the release notes for debug info metadata changes, since it's a very specialized topic — that doesn't mean you have to follow my bad example, of course ;-)
/// \param File Source file of the module declaration. Used for Fortran modules.