Page MenuHomePhabricator

[AsmParser] Add source location to all errors related to .cfi directives
ClosedPublic

Authored by arichardson on Oct 20 2020, 4:22 AM.

Details

Summary

I was trying to add .cfi_ annotations to assembly code in the FreeBSD
kernel and changed a macro that then resulted in incorrectly nested
directives. However, clang's diagnostics said the error was happening at
<unknown>:0. This addresses one of the TODOs added in D51695.

Diff Detail

Event Timeline

arichardson created this revision.Oct 20 2020, 4:22 AM
arichardson requested review of this revision.Oct 20 2020, 4:22 AM
MaskRay added inline comments.
llvm/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp
245 ↗(On Diff #301927)

These can be simplified by switching to default arguments in the declarations.

arichardson added inline comments.Fri, Oct 30, 10:01 AM
llvm/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp
245 ↗(On Diff #301927)

Yes. However, I chose to not add default arguments to ensure that all callers from the asmparser side are forced to pass the argument. I initially had default arguments but missed a lot of call sites before I removed the default.

arichardson added inline comments.Fri, Oct 30, 10:06 AM
llvm/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp
245 ↗(On Diff #301927)

To simplify the cases where there is no Loc, we could do something like void emitCFIEndProc(MCSTREAMER_SMLOC(Loc));

The files that don't can't to pass a SMLoc could then do
#define MCSTREAMER_SMLOC(name) SMLoc name = SMLoc()
but I'm not sure this is worth the effort.

rnk added a comment.Fri, Oct 30, 3:13 PM

I've been thinking about this problem of missing SMLocs for a while, and I think the right direction is probably for MCStreamer to have a way to get the location of the beginning of the directive being processed. That would suffice for the majority of these .cfi_* directives. It would make it much easier to get error reporting right by default or to add in error reporting later.

In D89787#2365791, @rnk wrote:

I've been thinking about this problem of missing SMLocs for a while, and I think the right direction is probably for MCStreamer to have a way to get the location of the beginning of the directive being processed. That would suffice for the majority of these .cfi_* directives. It would make it much easier to get error reporting right by default or to add in error reporting later.

How should MCStreamer get an access to the parser's SMLoc? The constructor of AsmParser has a non-const reference to MCStreamer. Add a SMLoc pointer in MCStreamer and make it aware of the parser's SMLoc in the constructor of AsmParser?

rnk added a comment.Fri, Oct 30, 3:30 PM
In D89787#2365791, @rnk wrote:

I've been thinking about this problem of missing SMLocs for a while, and I think the right direction is probably for MCStreamer to have a way to get the location of the beginning of the directive being processed. That would suffice for the majority of these .cfi_* directives. It would make it much easier to get error reporting right by default or to add in error reporting later.

How should MCStreamer get an access to the parser's SMLoc? The constructor of AsmParser has a non-const reference to MCStreamer. Add a SMLoc pointer in MCStreamer and make it aware of the parser's SMLoc in the constructor of AsmParser?

Sounds reasonable to me. Note, it's not the current parser location, it's the saved location of the start of the directive, but otherwise, that's the idea.

In D89787#2365826, @rnk wrote:
In D89787#2365791, @rnk wrote:

I've been thinking about this problem of missing SMLocs for a while, and I think the right direction is probably for MCStreamer to have a way to get the location of the beginning of the directive being processed. That would suffice for the majority of these .cfi_* directives. It would make it much easier to get error reporting right by default or to add in error reporting later.

How should MCStreamer get an access to the parser's SMLoc? The constructor of AsmParser has a non-const reference to MCStreamer. Add a SMLoc pointer in MCStreamer and make it aware of the parser's SMLoc in the constructor of AsmParser?

Sounds reasonable to me. Note, it's not the current parser location, it's the saved location of the start of the directive, but otherwise, that's the idea.

OK, D90511 for the idea.

(I confess I committed a symbol binding related SMLoc change abruptly yesterday.. Sorry)

arichardson edited the summary of this revision. (Show Details)Wed, Nov 4, 1:12 PM
arichardson added a reviewer: MaskRay.
MaskRay added inline comments.Wed, Nov 4, 1:17 PM
llvm/lib/MC/MCStreamer.cpp
970

Perhaps EndLoc is clearer.

llvm/test/MC/X86/cfi-open-within-another-crash.s
15–16

[[@LINE+x]] is a deprecated form. Use [[#@LINE-1]]

arichardson marked 2 inline comments as done.

address review feedback

MaskRay accepted this revision.Wed, Nov 4, 1:42 PM

LGTM. One last comment

llvm/test/MC/X86/cfi-scope-unclosed.s
0

If cfi-scope-errors.s includes the coverage perhaps delete the file

This revision is now accepted and ready to land.Wed, Nov 4, 1:42 PM
arichardson marked an inline comment as done.
  • Merge two test that don't need to be separate files
This revision was landed with ongoing or failed builds.Wed, Nov 11, 9:01 AM
This revision was automatically updated to reflect the committed changes.