[PATCH][MC] Emit an error if cfi_startproc is used before a symbol is defined
Quentin Colombet
qcolombet at apple.com
Mon Apr 14 14:29:59 PDT 2014
Thanks Eric for the analysis!
I’ll update the patch with your comments and I’ll file a PR.
-Quentin
On Apr 14, 2014, at 2:27 PM, Eric Christopher <echristo at gmail.com> wrote:
> On Mon, Apr 14, 2014 at 2:25 PM, Jim Grosbach <grosbach at apple.com> wrote:
>> Wow. That’s special.
>>
>> Does anyone have actual use-cases where extant code is relying on this behavior? I’m inclined to go with Quentin’s proposal and then if we run into cases later (like this sort of stuff) that require being more lenient, we fix it then. I don’t want to build in additional complexity like this just for the sake of doing the same thing as gas(1). If we were seeing code that used it now, we’d be seeing a crash just like what inspired the proposal in the first place.
>>
>
> *shrug* I can't think of anything offhand.
>
> Otherwise, one nit on the patch:
>
> + // Abort if LastSymbol is not defined.
> + // The start of the frame could not be bound to any location.
> + if (!LastSymbol)
> + report_fatal_error("No symbol to start a frame");
>
> We're not aborting and this scans bad. How about:
>
> "Report an error if we haven't seen a symbol yet where we'd bind
> .cfi_startproc."
>
> -eric
>
>> -Jim
>>
>> On Apr 14, 2014, at 2:05 PM, Eric Christopher <echristo at gmail.com> wrote:
>>
>>> gas appears to create a symbol for the .cfi_startproc and makes the
>>> relocation relative to it:
>>>
>>> .text
>>> .space 1000
>>> .cfi_startproc
>>> .space 1000
>>> .globl _someFunction
>>> _someFunction :
>>> .cfi_def_cfa_offset 16
>>> .cfi_offset %rbp,-16
>>> .cfi_def_cfa_register rbp
>>> ret
>>> .cfi_endproc
>>>
>>> RELOCATION RECORDS FOR [.eh_frame]:
>>> OFFSET TYPE VALUE
>>> 0000000000000020 R_X86_64_PC32 .text+0x00000000000003e8
>>>
>>> which holds up if you do:
>>>
>>> .text
>>> .space 1000
>>> .globl _someFunction
>>> _someFunction :
>>> .space 1000
>>> .cfi_startproc
>>> .cfi_def_cfa_offset 16
>>> .cfi_offset %rbp,-16
>>> .cfi_def_cfa_register rbp
>>> ret
>>> .cfi_endproc
>>>
>>> RELOCATION RECORDS FOR [.eh_frame]:
>>> OFFSET TYPE VALUE
>>> 0000000000000024 R_X86_64_PC32 .text+0x00000000000007d0
>>>
>>> which seems to say that to be compatible we'll need to do the
>>> relocation for the .cfi_startproc we need to emit it at the location
>>> of the directive and not at the last known symbol.
>>>
>>> What possible use this has? I have no idea. The last one appears that
>>> it could be useful in some ways. Define a chunk of space that isn't
>>> part of your function at the top and so you don't want it to be
>>> considered part of the call frame that you then patch in on start up
>>> perhaps?
>>>
>>> *shrug*
>>>
>>> Thoughts from the peanut gallery?
>>>
>>> -eric
>>>
>>> On Wed, Apr 9, 2014 at 4:06 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>>>> Hi Eric and Jim,
>>>>
>>>> @Eric, I do not know if you are the right person to review this, but compact
>>>> frame information looks related enough to dwarf emission that I assume you
>>>> know :).
>>>>
>>>> The attached patch fixes an assertion failure[1] in the assembler when a
>>>> .cfi_startproc is used before a function (or more generally a symbol) is
>>>> defined.
>>>>
>>>> Thanks for your reviews/feedbacks.
>>>>
>>>> ** Context **
>>>>
>>>> Currently the assembler crashes when emitting the compact unwind information
>>>> if the input assembly file starts with a .cfi_startproc directive before any
>>>> symbol has been defined.
>>>>
>>>> I am not a CFI expert, but that does not look right to me to use a
>>>> .cfi_startproc directive without having started the related function!
>>>>
>>>>
>>>> ** Proposed Solution **
>>>>
>>>> Issue a backend fatal error if a frame is started whereas no symbol is
>>>> defined. Although this diagnostic method is not pretty, this is how all cfi
>>>> related errors are handled (e.g., looks for No open frame).
>>>>
>>>> The patch also modifies a few test cases to define a function before
>>>> .cfi_startproc is used.
>>>> I.e., test cases like this:
>>>> .cfi_startproc
>>>>
>>>> Looks like this now:
>>>> _proc:
>>>> .cfi_startproc
>>>>
>>>> If that does not make sense, please let me know!
>>>>
>>>> <rdar://problem/15939159>
>>>>
>>>> [1]
>>>> Assertion failed: (Symbol), function MCSymbolRefExpr, file
>>>> include/llvm/MC/MCExpr.h, line 273.
>>>> 0 llvm-mc 0x0000000109d85e3e
>>>> llvm::sys::PrintStackTrace(__sFILE*) + 46
>>>> 1 llvm-mc 0x0000000109d8614b
>>>> PrintStackTraceSignalHandler(void*) + 27
>>>> 2 llvm-mc 0x0000000109d864c8 SignalHandler(int) + 408
>>>> 3 libsystem_platform.dylib 0x00007fff8f8895aa _sigtramp + 26
>>>> 4 libsystem_platform.dylib 0x00007f9452c05338 _sigtramp + 3275210152
>>>> 5 llvm-mc 0x0000000109d8617b raise + 27
>>>> 6 llvm-mc 0x0000000109d86232 abort + 18
>>>> 7 llvm-mc 0x0000000109d86211 __assert_rtn + 129
>>>> 8 llvm-mc 0x0000000109d019bf
>>>> llvm::MCSymbolRefExpr::MCSymbolRefExpr(llvm::MCSymbol const*,
>>>> llvm::MCSymbolRefExpr::VariantKind, llvm::MCAsmInfo const*) + 143
>>>> 9 llvm-mc 0x0000000109d0081b
>>>> llvm::MCSymbolRefExpr::MCSymbolRefExpr(llvm::MCSymbol const*,
>>>> llvm::MCSymbolRefExpr::VariantKind, llvm::MCAsmInfo const*) + 43
>>>> 10 llvm-mc 0x0000000109cfda56
>>>> llvm::MCSymbolRefExpr::Create(llvm::MCSymbol const*,
>>>> llvm::MCSymbolRefExpr::VariantKind, llvm::MCContext&) + 134
>>>> 11 llvm-mc 0x0000000109b79df4
>>>> llvm::MCSymbolRefExpr::Create(llvm::MCSymbol const*, llvm::MCContext&) + 36
>>>> 12 llvm-mc 0x0000000109d16f85
>>>> llvm::MCStreamer::EmitSymbolValue(llvm::MCSymbol const*, unsigned int) + 69
>>>> 13 llvm-mc 0x0000000109ceb7ca (anonymous
>>>> namespace)::FrameEmitterImpl::EmitCompactUnwind(llvm::MCStreamer&,
>>>> llvm::MCDwarfFrameInfo const&) + 314
>>>> 14 llvm-mc 0x0000000109ceb2d6
>>>> llvm::MCDwarfFrameEmitter::Emit(llvm::MCStreamer&, llvm::MCAsmBackend*,
>>>> bool, bool) + 470
>>>> 15 llvm-mc 0x0000000109d1b1cf
>>>> llvm::MCStreamer::EmitFrames(llvm::MCAsmBackend*, bool) + 95
>>>> 16 llvm-mc 0x0000000109d04152 (anonymous
>>>> namespace)::MCMachOStreamer::FinishImpl() + 66
>>>> 17 llvm-mc 0x0000000109d1b2df llvm::MCStreamer::Finish() +
>>>> 159
>>>> 18 llvm-mc 0x0000000109c30582 (anonymous
>>>> namespace)::AsmParser::Run(bool, bool) + 2546
>>>> 19 llvm-mc 0x000000010995457c AssembleInput(char const*,
>>>> llvm::Target const*, llvm::SourceMgr&, llvm::MCContext&, llvm::MCStreamer&,
>>>> llvm::MCAsmInfo&, llvm::MCSubtargetInfo&, llvm::MCInstrInfo&) + 940
>>>> 20 llvm-mc 0x0000000109952461 main + 9457
>>>> 21 libdyld.dylib 0x00007fff959e65fd start + 1
>>>>
>>>> Cheers,
>>>> -Quentin
>>>>
>>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140414/3398687f/attachment.html>
More information about the llvm-commits
mailing list