Page MenuHomePhabricator

[Assembler] Enable nicer diagnostics for inline assembly.
ClosedPublic

Authored by sanwou01 on Feb 2 2017, 1:30 AM.

Details

Summary

Enables source location in diagnostic messages from the backend. This
is after parsing, during finalization. This requires the SourceMgr, the
inline assembly string buffer, and DiagInfo to still be alive after
EmitInlineAsm returns.

This patch creates a single SourceMgr for inline assembly inside the
AsmPrinter. MCContext gets a pointer to this SourceMgr. Using one
SourceMgr per call to EmitInlineAsm would make it difficult for
MCContext to figure out in which SourceMgr the SMLoc is located, while a
single SourceMgr can figure it out if it has multiple buffers.

The Str argument to EmitInlineAsm is copied into a buffer and owned by
the inline asm SourceMgr. This ensures that DiagHandlers won't print
garbage. (Clang emits a "note: instantiated into assembly here", which
refers to this string.)

The AsmParser gets destroyed before finalization, which means that the
DiagHandlers the AsmParser installs into the SourceMgr will be stale.
Restore the saved DiagHandlers.

Since now we're using just one SourceMgr for multiple inline asm
strings, we need to tell the AsmParser which buffer it needs to parse
currently. Hand a buffer id -- returned from SourceMgr::
AddNewSourceBuffer -- to the AsmParser.

Diff Detail

Repository
rL LLVM

Event Timeline

sanwou01 created this revision.Feb 2 2017, 1:30 AM

These are patched from D29409, D29410, D29411, D29412, D29413, D29414 combined.

I have to agree with @rengolin that it was easier to write the rationale for the combined patch. Thanks for your patience while I figure out what the right scope for a single patch is. :-)

rengolin edited edge metadata.

Thanks! Much easier to review. :)

Adding a few more folks.

include/llvm/CodeGen/AsmPrinter.h
114 ↗(On Diff #86779)

I'd move this line down, as it's being handled by the new methods there.

anemet edited edge metadata.Feb 2 2017, 9:15 AM

Probably a completely silly question but why don't we use LLVMContext::InlineAsmDiagHandler for this?

rnk added inline comments.Feb 2 2017, 9:31 AM
include/llvm/CodeGen/AsmPrinter.h
114 ↗(On Diff #86779)

Most classes seem to group fields together separately from methods, so I think this is fine.

lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
105 ↗(On Diff #86779)

Why do we need a collection of new DiagInfos for every inline asm buffer? It seems like we can have just one in place of the vector on AsmPrinter, and then let the assignments below rewrite it for every inline asm blob we process. In fact, now we don't need to set the source manager diagnostic handler every time we process an inline asm blob. We could do it when we create the source manager instead.

116–117 ↗(On Diff #86779)

I'd reword this comment along these lines to help explain the lifetime issues:

The inline asm source manager will outlive Str, so make a copy of the string for SourceMgr to own.
rengolin added inline comments.Feb 2 2017, 9:34 AM
include/llvm/CodeGen/AsmPrinter.h
114 ↗(On Diff #86779)

Ok for me, then maybe bring DiagInfos up here and add a comment?

Probably a completely silly question but why don't we use LLVMContext::InlineAsmDiagHandler for this?

The diagnostics callback will pass through the DiagHandler saved in LLVMContext already. It's the one saved in DiagInfo, and called through the local DiagHandler. This patch adds plumbing for diagnostics to work after parsing, from the backend.

include/llvm/CodeGen/AsmPrinter.h
114 ↗(On Diff #86779)

I'll move DiagInfos (or singular, see below) up here, thanks.

lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
105 ↗(On Diff #86779)

My concern is with LocMDNode, which is used to generate a LocCookie in the diagHandler if it's non-null. This could presumably be different for different calls to EmitInlineAsm, and we would overwrite that.

That said, there are no guarantees that the LocMDNode will still be alive during finalization. Having one DiagInfo, like you suggested, and resetting LocMDNode to a nullptr after parsing seems to me like the way to go. Agreed?

116–117 ↗(On Diff #86779)

Will do, thanks.

sanwou01 updated this revision to Diff 87428.Feb 7 2017, 7:11 AM

Address reviewers' comments.

I took the opportunity to clean up the initialisation of DiagInfo (which now
includes the SourceMgr). As suggested by @rnk the values in DiagInfo (except
for LocInfo) are set only once.

rnk accepted this revision.Feb 7 2017, 12:44 PM

lgtm

This revision is now accepted and ready to land.Feb 7 2017, 12:44 PM
This revision was automatically updated to reflect the committed changes.