Page MenuHomePhabricator

[DebugInfo] Fortran module DebugInfo support in LLVM
ClosedPublic

Authored by SouraVX on Wed, May 6, 5:35 AM.

Details

Summary

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.

Diff Detail

Event Timeline

SouraVX created this revision.Wed, May 6, 5:35 AM
Herald added a project: Restricted Project. · View Herald Transcript

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.
LineNo = 0?

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
SouraVX added inline comments.Wed, May 6, 11:19 AM
llvm/include/llvm/IR/DebugInfoMetadata.h
2139

Didn't get you completely here, I think that's needed, if you're asking.
Is there better way to accomplish this ? I remember noting some failures when using parent DIScope getFile.

DavidTruby resigned from this revision.Wed, May 6, 12:06 PM
SouraVX updated this revision to Diff 262465.Wed, May 6, 1:34 PM

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.
SouraVX marked 7 inline comments as done.Wed, May 6, 1:37 PM
SouraVX added inline 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.

SouraVX marked an inline comment as done.Thu, May 7, 9:58 AM

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.

SouraVX added inline comments.Thu, May 7, 10:01 AM
llvm/lib/Bitcode/Reader/MetadataLoader.cpp
1421

Sorry! didn't get you exactly. Two fields 6, 7 are added to capture file, line info.

aprantl added inline comments.Thu, May 7, 10:31 AM
llvm/include/llvm/IR/DebugInfoMetadata.h
2139

I remember noting some failures when using parent DIScope getFile.

We'll have to figure that out first.
Looking at the definition of getRawFile() in DIScope, it expects the File to be Operand[0]. Did you take this into account? I think if you change the order of metadata operands to have the File first, you should be able to just use the parent DIScope's getFile functionality. Is that perhaps why it didn't work when you first tried it?

SouraVX updated this revision to Diff 262754.Thu, May 7, 2:05 PM

Addressed @aprantl review comments, Thanks for this!

  • Made DIFile as first metadata operand and removed getFile from DIModule, now it's accessible via DIScope.
SouraVX marked an inline comment as done.Thu, May 7, 2:05 PM
aprantl added inline comments.Fri, May 8, 9:25 AM
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)

SouraVX updated this revision to Diff 262895.Fri, May 8, 10:39 AM

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.
SouraVX marked 12 inline comments as done.Fri, May 8, 10:43 AM
SouraVX added inline comments.
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.
I didn't noticed any regression failure after this.

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!

aprantl added inline comments.Mon, May 11, 10:26 AM
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.

SouraVX updated this revision to Diff 263224.Mon, May 11, 11:25 AM
SouraVX marked 3 inline comments as done.

Addressed @aprantl comments, Thanks for this!

SouraVX marked 2 inline comments as done.Mon, May 11, 11:26 AM
SouraVX added inline comments.
llvm/lib/Bitcode/Reader/MetadataLoader.cpp
1431

Thanks! for the suggestion. Code seems more robust and consistent now.

SouraVX marked an inline comment as done.Mon, May 11, 11:28 AM
aprantl added inline comments.Mon, May 11, 7:28 PM
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

djtodoro added inline comments.
llvm/lib/IR/DebugInfoMetadata.cpp
744

Is this clang-formated ?

SouraVX marked 2 inline comments as done.Tue, May 12, 2:03 AM
SouraVX added inline comments.
llvm/lib/IR/DebugInfoMetadata.cpp
744

Yes!

SouraVX updated this revision to Diff 263397.Tue, May 12, 4:50 AM
SouraVX marked an inline comment as done.

Addressed @aprantl comments, Thanks for this!

  • Now Bitcode backward/forward compatibility is maintained.
SouraVX marked 3 inline comments as done.Tue, May 12, 4:55 AM
SouraVX added inline comments.
llvm/lib/Bitcode/Reader/MetadataLoader.cpp
1431

Thanks for pointing this out!
That was indeed the problem, test case .bc file DIModule-clang-module.ll.bc was previously generated using modified( i.e after this patch)llvm-as, that's why the issue doesn't surfaced.

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.

aprantl accepted this revision.Tue, May 12, 1:02 PM

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 revision is now accepted and ready to land.Tue, May 12, 1:02 PM
This revision was automatically updated to reflect the committed changes.
SouraVX marked an inline comment as done.

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.

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

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 ;-)

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 ;-)

Done!, please review D80614. Thanks!