Page MenuHomePhabricator

[MC] Make MCStreamer aware of AsmParser's StartTokLoc
ClosedPublic

Authored by MaskRay on Oct 30 2020, 4:02 PM.

Details

Summary

A SMLoc allows MCStreamer to report location-aware diagnostics, which
were previously done by adding SMLoc to various methods (e.g. emit*) in an ad-hoc way.

Since the file:line is most important, the column is less important and
the start token location suffices in many cases, this patch reverts
b7e7131af2dd7bdb03fa42a3bc1b4bc72ab95ce1

// old
symbol-binding-changed.s:6:8: error: local changed binding to STB_GLOBAL
.globl local
       ^
// new
symbol-binding-changed.s:6:1: error: local changed binding to STB_GLOBAL
.globl local
^

Diff Detail

Event Timeline

MaskRay created this revision.Oct 30 2020, 4:02 PM
MaskRay requested review of this revision.Oct 30 2020, 4:02 PM
arichardson added a subscriber: arichardson.EditedOct 30 2020, 4:21 PM

This seems like a good solution to the problem I'm trying to fix in D89787 and I'll rebase on top of this. Not sure about the name StartTokLocPtr though. Maybe CurrentParserLoc?

llvm/lib/MC/MCParser/AsmParser.cpp
1713

This seems like a good solution to the problem I'm trying to fix in D89787 and I'll rebase on top of this. Not sure about the name StartTokLocPtr though. Maybe CurrentParserLoc?

AsmParser.cpp:1919 has a variable called StartTokLoc. I felt the usage is similar so I picked it. We store the location of the starting token, instead of the current token, so CurrentParserLoc might be inappropriate.

rnk accepted this revision.Nov 2 2020, 12:21 PM

Thanks, looks good to me.

This revision is now accepted and ready to land.Nov 2 2020, 12:21 PM
This revision was landed with ongoing or failed builds.Nov 2 2020, 12:32 PM
This revision was automatically updated to reflect the committed changes.

It was an pre-existing problem which has been fixed by bc847b31435e48ad0e54b322a716a4b9f07bc232

It was an pre-existing problem which has been fixed by bc847b31435e48ad0e54b322a716a4b9f07bc232

It's still broken

MaskRay added a comment.EditedNov 3 2020, 11:51 AM

It was an pre-existing problem which has been fixed by bc847b31435e48ad0e54b322a716a4b9f07bc232

It's still broken

Did you mean http://lab.llvm.org:8011/#/builders/99/builds/422 ? I think it is due to a different patch. I am going to investigate it

Failed Tests (1):
  Clang-Unit :: Tooling/./ToolingTests/TransformerTest.TemplateInstantiation

==51632==ERROR: AddressSanitizer: stack-use-after-return on address 0x7f0becfa8b00 at pc 0x00000ae0212e bp 0x7ffe5a83f8b0 sp 0x7ffe5a83f8a8
READ of size 8 at 0x7f0becfa8b00 thread T0
    #0 0xae0212d in onStartOfTranslationUnit /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/clang/lib/ASTMatchers/ASTMatchFinder.cpp:391:11
    #1 0xae0212d in clang::ast_matchers::MatchFinder::matchAST(clang::ASTContext&) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/clang/lib/ASTMatchers/ASTMatchFinder.cpp:1234:11
    #2 0xecb767e in clang::ParseAST(clang::Sema&, bool, bool) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/clang/lib/Parse/ParseAST.cpp:171:13
    #3 0xb3e7ab4 in clang::FrontendAction::Execute() /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/clang/lib/Frontend/FrontendAction.cpp:949:8
`

It was an pre-existing problem which has been fixed by bc847b31435e48ad0e54b322a716a4b9f07bc232

It's still broken

Did you mean http://lab.llvm.org:8011/#/builders/99/builds/422 ? I think it is due to a different patch. I am going to investigate it

Failed Tests (1):
  Clang-Unit :: Tooling/./ToolingTests/TransformerTest.TemplateInstantiation

==51632==ERROR: AddressSanitizer: stack-use-after-return on address 0x7f0becfa8b00 at pc 0x00000ae0212e bp 0x7ffe5a83f8b0 sp 0x7ffe5a83f8a8
READ of size 8 at 0x7f0becfa8b00 thread T0
    #0 0xae0212d in onStartOfTranslationUnit /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/clang/lib/ASTMatchers/ASTMatchFinder.cpp:391:11
    #1 0xae0212d in clang::ast_matchers::MatchFinder::matchAST(clang::ASTContext&) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/clang/lib/ASTMatchers/ASTMatchFinder.cpp:1234:11
    #2 0xecb767e in clang::ParseAST(clang::Sema&, bool, bool) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/clang/lib/Parse/ParseAST.cpp:171:13
    #3 0xb3e7ab4 in clang::FrontendAction::Execute() /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/clang/lib/Frontend/FrontendAction.cpp:949:8
`

@vitalybuka Fixed ASAN_OPTIONS=detect_stack_use_after_return=1 ..../tools/clang/unittests/Tooling/ToolingTests --gtest_filter='*TransformerTest.TemplateInstantiation' in 96ed6793b35e8267b0c94ebe69ae94f07024f476. The relevant code was last touched in 2019 so I wonder why it only failed recently...