Page MenuHomePhabricator

[Flang] Add/Restore basic debug support (1/n)
ClosedPublic

Authored by kiranchandramohan on Nov 14 2022, 8:06 AM.

Details

Summary

Recent changes to MLIR meant that Flang does not generate any debug line
table information.

This patch adds a pass that provides some foundation work with which
basic line table debug info can be generated. A walk is performed on
all the func ops in the module and they are decorated with a fusedLoc
op that contains the debug metadata for the subroutine along with
location information.

Alternatives include populating this info during lowering or during FIR
to LLVM Dialect conversion.

Note: Patches in future will add

-> more realistic debug info for types and other fields.
-> driver flags to control generation of debug.

Fixes #58634.

Diff Detail

Event Timeline

Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
kiranchandramohan requested review of this revision.Nov 14 2022, 8:06 AM
tblah added a subscriber: tblah.Nov 18 2022, 2:33 AM

Minor fixes, format and rebase.

In https://reviews.llvm.org/D140415 there was similar question: pass or part of lowering? Could you register callbacks or some other magic into the lowering part to make it extensible for debug info, stack arrays, and more?

Thank you, Kiran! It looks good to me, except for the handling of file name/path.

flang/lib/Frontend/FrontendActions.cpp
545

I think it would be better to take the file name/path from the ModuleOp itself (e.g. via module.getLoc()). I know there is nothing ready for linking of multiple modules with FIR, but we would probably want to do this at some point. Thus, we'd better try to host all the needed information in the module itself. Setting the module source location should be easy, e.g. pass the input file name to LoweringBridge::create. I am not sure what we should do for MLIR input files - probably, preserve the source location read from MLIR and if it is empty, use the MLIR file location instead.

Can you please try this?

vzakhari added inline comments.Fri, Jan 6, 2:59 PM
flang/lib/Frontend/FrontendActions.cpp
545

Actually, the path information seems to be already available via SemanticsContext.

In https://reviews.llvm.org/D140415 there was similar question: pass or part of lowering? Could you register callbacks or some other magic into the lowering part to make it extensible for debug info, stack arrays, and more?

We discussed that a bit in the RFC and can discuss more there. We also need to sync with what the MLIR team has in mind. In the mean-time, making some progress here is probably more beneficial (for engineers working on performance improvement) at this point than coming up with the perfect solution.

flang/lib/Frontend/FrontendActions.cpp
545

Sure. I am trying this.

Did you mean using the function semanticsContext.location()? That field is not set at this point in LowerBridge::create.

vzakhari added inline comments.Mon, Jan 9, 9:10 AM
flang/lib/Frontend/FrontendActions.cpp
545

Yes, I thought we could use it, but I did not track it down completely.

Address review comments. Add test.

kiranchandramohan retitled this revision from [Flang] WIP : Add/Restore basic debug support to [Flang] Add/Restore basic debug support (1/n).Tue, Jan 10, 6:10 PM
kiranchandramohan edited the summary of this revision. (Show Details)
vzakhari accepted this revision.Tue, Jan 10, 7:05 PM

Thank you, Kiran! It looks good to me.

flang/lib/Lower/Bridge.cpp
3827

nit: line 0, column 0 might be a better choice to indicate the beginning of the module (e.g. 0, 0 is used by the parser in mlir/lib/Parser/Parser.cpp). I guess 1, 1 may work as well.

This revision is now accepted and ready to land.Tue, Jan 10, 7:05 PM
awarzynski added inline comments.Wed, Jan 11, 12:15 AM
flang/lib/Frontend/FrontendActions.cpp
174–175

Wouldn't llvm::StringRef be sufficient here? As in, filePath{getCurrentFileOrBufferName()} invoked in-place.

flang/lib/Frontend/FrontendActions.cpp
174–175

Do you mean passing std::string(getCurrentFileOrBufferName()) instead of creating a string with a name filePath? Or do you mean whether passing getCurrentFileOrBufferName() is fine?

The former is fine. For the latter, I was not sure about the lifetime of the underlying string of getCurrentFileOrBufferName(). Is that guaranteed to exist till the MLIR flow is complete? All the places following from here uses StringRef.

flang/lib/Optimizer/Transforms/AddDebugFoundation.cpp
2

Nit: Change comment to match file.

58

A conditional check is probably required here.

64–66

Provide comments for any parameter without a name or type.

Address comments about directly passing StringRef and using 0,0 as default location.

kiranchandramohan marked 2 inline comments as done.Wed, Jan 11, 9:27 AM
kiranchandramohan added inline comments.
flang/lib/Frontend/FrontendActions.cpp
174–175

Passing it directly now. Done.

flang/lib/Lower/Bridge.cpp
3827

Made the change. Thanks for the pointer.

awarzynski accepted this revision.Thu, Jan 12, 12:51 AM

Thanks Kiran! LGTM

flang/lib/Frontend/FrontendActions.cpp
174–175

For the latter, I was not sure about the lifetime of the underlying string of getCurrentFileOrBufferName(). Is that guaranteed to exist till the MLIR flow is complete?

That string is managed by the driver. As long as the driver is "on", that string is guaranteed to exist. And the driver is guaranteed to be "on" until the end of the compilation :)

This revision was automatically updated to reflect the committed changes.
kiranchandramohan marked an inline comment as done.

@kiranchandramohan, I notice that the path being passed to LoweringBridge.create is a relative path. Is this what you wanted?

@kiranchandramohan, I notice that the path being passed to LoweringBridge.create is a relative path. Is this what you wanted?

Thanks for bringing this up. I think we need the full path for the debugger to find the file. I will check whether the full path is available and provide an update.

vzakhari added inline comments.Thu, Jan 12, 9:45 AM
flang/lib/Optimizer/Transforms/AddDebugFoundation.cpp
80

Jean mentinoned the preprocessing include directives that might bring code from other files - in this case, I think, we cannot use fileAttr here. We need to create new DIFileAttr based on the actual location of the FuncOp.

flang/lib/Optimizer/Transforms/AddDebugFoundation.cpp
80

Thanks @vzakhari. I will make the suggested change.

klausler added a subscriber: klausler.EditedThu, Jan 12, 10:54 AM

How to get the initial source file path from the Fortran::semantics::SemanticsContext instance:

std::optional<std::string> path;
const auto &allSources{semanticsContext.allCookedSources().allSources()};
if (auto initial{allSources.GetFirstFileProvenance()};
    initial && !initial->empty()) {
  if (const auto *sourceFile{allSources.GetSourceFile(initial->begin())}) {
    path = sourceFile->path();
  }
}

How to get the initial source file path from the Fortran::semantics::SemanticsContext instance:

std::optional<std::string> path;
const auto &allSources{semanticsContext.allCookedSources().allSources()};
if (auto initial{allSources.GetFirstFileProvenance()};
    initial && !initial->empty()) {
  if (const auto *sourceFile{allSources.GetSourceFile(initial->begin())}) {
    path = sourceFile->path();
  }
}

Thanks Peter.