Skip to content

Commit 1a41a11

Browse files
author
Kristina Brooks
committedOct 19, 2018
[MC][DWARF][AsmParser] Ensure nested CFI frames are diagnosed.
This avoids a crash (with asserts) or bad codegen (without asserts) in Dwarf streamer later on. This patch fixes this condition in MCStreamer and propogates SMLoc down when it's available with an added bonus of source locations for those specific types of errors. Further patches could use similar improvements as currently most non-Windows CFI directives lack an SMLoc parameter. Modified an existing test to verify source location propogation and added an object-file version of it to verify that it does not crash in addition to a standalone test to only ensure it does not crash. Differential Revision: https://reviews.llvm.org/D51695 llvm-svn: 344781
1 parent 22bad04 commit 1a41a11

File tree

5 files changed

+40
-10
lines changed

5 files changed

+40
-10
lines changed
 

‎llvm/include/llvm/MC/MCStreamer.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -870,7 +870,7 @@ class MCStreamer {
870870

871871
virtual MCSymbol *getDwarfLineTableSymbol(unsigned CUID);
872872
virtual void EmitCFISections(bool EH, bool Debug);
873-
void EmitCFIStartProc(bool IsSimple);
873+
void EmitCFIStartProc(bool IsSimple, SMLoc Loc = SMLoc());
874874
void EmitCFIEndProc();
875875
virtual void EmitCFIDefCfa(int64_t Register, int64_t Offset);
876876
virtual void EmitCFIDefCfaOffset(int64_t Offset);

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

+7-2
Original file line numberDiff line numberDiff line change
@@ -3919,8 +3919,13 @@ bool AsmParser::parseDirectiveCFIStartProc() {
39193919
parseToken(AsmToken::EndOfStatement))
39203920
return addErrorSuffix(" in '.cfi_startproc' directive");
39213921
}
3922-
3923-
getStreamer().EmitCFIStartProc(!Simple.empty());
3922+
3923+
// TODO(kristina): Deal with a corner case of incorrect diagnostic context
3924+
// being produced if this directive is emitted as part of preprocessor macro
3925+
// expansion which can *ONLY* happen if Clang's cc1as is the API consumer.
3926+
// Tools like llvm-mc on the other hand are not affected by it, and report
3927+
// correct context information.
3928+
getStreamer().EmitCFIStartProc(!Simple.empty(), Lexer.getLoc());
39243929
return false;
39253930
}
39263931

‎llvm/lib/MC/MCStreamer.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -347,10 +347,10 @@ void MCStreamer::EmitCFISections(bool EH, bool Debug) {
347347
assert(EH || Debug);
348348
}
349349

350-
void MCStreamer::EmitCFIStartProc(bool IsSimple) {
350+
void MCStreamer::EmitCFIStartProc(bool IsSimple, SMLoc Loc) {
351351
if (hasUnfinishedDwarfFrameInfo())
352-
getContext().reportError(
353-
SMLoc(), "starting new .cfi frame before finishing the previous one");
352+
return getContext().reportError(
353+
Loc, "starting new .cfi frame before finishing the previous one");
354354

355355
MCDwarfFrameInfo Frame;
356356
Frame.IsSimple = IsSimple;
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
# Test for D51695 ensuring there is no crash when two .cfi_startproc are opened
2+
# without the first one being closed.
3+
4+
# RUN: not llvm-mc %s -filetype=obj -triple=x86_64-unknown-linux -o /dev/null 2>&1 | FileCheck %s
5+
6+
.text
7+
.globl proc_one
8+
proc_one:
9+
.cfi_startproc
10+
11+
.text
12+
.globl proc_two
13+
proc_two:
14+
.cfi_startproc
15+
16+
.cfi_endproc
17+
18+
# CHECK: error: starting new .cfi frame before finishing the previous one

‎llvm/test/MC/X86/cfi-scope-errors.s

+11-4
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
1-
# RUN: not llvm-mc %s -triple x86_64-linux -o /dev/null 2>&1 | FileCheck %s --implicit-check-not=error:
2-
3-
# FIXME: Push source locations into diagnostics.
1+
# RUN: not llvm-mc %s -triple x86_64-linux -o /dev/null 2>&1 | FileCheck %s
2+
# RUN: not llvm-mc %s -triple x86_64-linux -filetype=obj -o /dev/null 2>&1 | FileCheck %s
43

54
.text
65
.cfi_def_cfa rsp, 8
@@ -9,8 +8,16 @@
98
.cfi_startproc
109
nop
1110

11+
# TODO(kristina): As Reid suggested, this now supports source locations as a side effect
12+
# of another patch aimed at fixing the crash that would occur here, however the other
13+
# ones do not unfortunately. Will address it in a further patch propogating SMLoc down to
14+
# other CFI directives at which point more LINE checks can be added to ensure proper source
15+
# location reporting.
16+
17+
# This tests source location correctness as well as the error and it not crashing.
18+
# CHECK: [[@LINE+2]]:1: error: starting new .cfi frame before finishing the previous one
1219
.cfi_startproc
13-
# CHECK: error: starting new .cfi frame before finishing the previous one
20+
1421
nop
1522
.cfi_endproc
1623

0 commit comments

Comments
 (0)