[PATCH 1/2] Always create unwind table entries for functions marked `uwtable`
Björn Steinbrink
bsteinbr at gmail.com
Mon Feb 17 01:49:04 PST 2014
Hi Rafael,
On 2014.02.16 18:28:48 -0500, Rafael Espíndola wrote:
> I am not sure this is the correct way to read uwtable.
>
>
> The original intension was for it to force the output of any
> information needed for unwinding only. Sometimes it is needed because
> of an abi. Sometimes it is needed because the user wants to make sure
> a profiler or debugger can unwind the stack past a call to it.
>
> With that interpretation, the GCC_except_table should never be
> modified based on uwtable.
My interpretation of uwtable is based on the change that is reverted in
my second patch, the documentation in Attributes.h and the existing
method called "needsUnwindTableEntry", which suggests that in fact an
entry in the table should be created. If that interpretation is wrong
and uwtable should just force CFI to be emitted, then this patch isn't
required, there's already code that checks uwtable to do that.
> What use case are we not capturing? I.E., what prevents you from just
> dropping nounwind from a function you want to see in GCC_except_table?
My goal was to restore the invoke->call optimization that was disabled
by the change I reverted in my second patch, so I tried to fix the
problem I thought that patch was fixing in a different way. But having
another look now, that patch actually had no effect on the case where an
unwind table is created anyway. It only forces the creation of unwind
tables with empty landing pads that would otherwise be completely
omitted. Maybe Bill can explain what the reason for that change was.
Björn
> On 16 February 2014 14:13, Björn Steinbrink <bsteinbr at gmail.com> wrote:
> > As explained in Attributes.h, functions marked `uwtable` should always
> > have an entry in the unwind table, even if they are additionally marked
> > `nounwind`.
> >
> > But the check in codegen only checks if the function is marked
> > `nounwind` and if so, it always omits the table entry, which is wrong.
> > The Function class already provides a method to correctly check if a
> > table entry is required. Using that instead of the `nounwind` check
> > fixes problem.
> > ---
> > lib/CodeGen/AsmPrinter/DwarfException.cpp | 40 +++++++++--------
> > lib/CodeGen/AsmPrinter/DwarfException.h | 6 +--
> > test/CodeGen/X86/gcc_except_table.ll | 73 +++++++++++++++++++++++++++++--
> > 3 files changed, 93 insertions(+), 26 deletions(-)
> >
> > diff --git a/lib/CodeGen/AsmPrinter/DwarfException.cpp b/lib/CodeGen/AsmPrinter/DwarfException.cpp
> > index f72bc81..4ff5599 100644
> > --- a/lib/CodeGen/AsmPrinter/DwarfException.cpp
> > +++ b/lib/CodeGen/AsmPrinter/DwarfException.cpp
> > @@ -181,12 +181,12 @@ ComputeActionsTable(const SmallVectorImpl<const LandingPadInfo*> &LandingPads,
> > return SizeActions;
> > }
> >
> > -/// CallToNoUnwindFunction - Return `true' if this is a call to a function
> > -/// marked `nounwind'. Return `false' otherwise.
> > -bool DwarfException::CallToNoUnwindFunction(const MachineInstr *MI) {
> > + /// CallNeedsUnwindTableEntry - Return `true' if this is a call to a function
> > + /// that needs an unwind table entry. Return `false' otherwise.
> > +bool DwarfException::CallNeedsUnwindTableEntry(const MachineInstr *MI) {
> > assert(MI->isCall() && "This should be a call instruction!");
> >
> > - bool MarkedNoUnwind = false;
> > + bool NeedsUnwindTableEntry = true;
> > bool SawFunc = false;
> >
> > for (unsigned I = 0, E = MI->getNumOperands(); I != E; ++I) {
> > @@ -204,15 +204,15 @@ bool DwarfException::CallToNoUnwindFunction(const MachineInstr *MI) {
> > //
> > // FIXME: Determine if there's a way to say that `F' is the callee or
> > // parameter.
> > - MarkedNoUnwind = false;
> > + NeedsUnwindTableEntry = true;
> > break;
> > }
> >
> > - MarkedNoUnwind = F->doesNotThrow();
> > + NeedsUnwindTableEntry = F->needsUnwindTableEntry();
> > SawFunc = true;
> > }
> >
> > - return MarkedNoUnwind;
> > + return NeedsUnwindTableEntry;
> > }
> >
> > /// ComputeCallSiteTable - Compute the call-site table. The entry for an invoke
> > @@ -230,9 +230,11 @@ ComputeCallSiteTable(SmallVectorImpl<CallSiteEntry> &CallSites,
> > // The end label of the previous invoke or nounwind try-range.
> > MCSymbol *LastLabel = 0;
> >
> > - // Whether there is a potentially throwing instruction (currently this means
> > - // an ordinary call) between the end of the previous try-range and now.
> > - bool SawPotentiallyThrowing = false;
> > + // Whether there is an instruction that forces us to emit unwind table
> > + // entries for calls between the end of the previous try-range and now.
> > + // Currently this means function calls that potentially throw or are marked
> > + // to always required an unwind table entry.
> > + bool ForceCallSiteEntry = false;
> >
> > // Whether the last CallSite entry was for an invoke.
> > bool PreviousIsInvoke = false;
> > @@ -244,14 +246,14 @@ ComputeCallSiteTable(SmallVectorImpl<CallSiteEntry> &CallSites,
> > MI != E; ++MI) {
> > if (!MI->isLabel()) {
> > if (MI->isCall())
> > - SawPotentiallyThrowing |= !CallToNoUnwindFunction(MI);
> > + ForceCallSiteEntry |= CallNeedsUnwindTableEntry(MI);
> > continue;
> > }
> >
> > // End of the previous try-range?
> > MCSymbol *BeginLabel = MI->getOperand(0).getMCSymbol();
> > if (BeginLabel == LastLabel)
> > - SawPotentiallyThrowing = false;
> > + ForceCallSiteEntry = false;
> >
> > // Beginning of a new try-range?
> > RangeMapType::const_iterator L = PadMap.find(BeginLabel);
> > @@ -265,10 +267,10 @@ ComputeCallSiteTable(SmallVectorImpl<CallSiteEntry> &CallSites,
> > "Inconsistent landing pad map!");
> >
> > // For Dwarf exception handling (SjLj handling doesn't use this). If some
> > - // instruction between the previous try-range and this one may throw,
> > - // create a call-site entry with no landing pad for the region between the
> > - // try-ranges.
> > - if (SawPotentiallyThrowing && Asm->MAI->isExceptionHandlingDwarf()) {
> > + // instruction between the previous try-range and this one requires an
> > + // unwind table entry create a call-site entry with no landing pad for
> > + // the region between the try-ranges.
> > + if (ForceCallSiteEntry && Asm->MAI->isExceptionHandlingDwarf()) {
> > CallSiteEntry Site = { LastLabel, BeginLabel, 0, 0 };
> > CallSites.push_back(Site);
> > PreviousIsInvoke = false;
> > @@ -316,9 +318,9 @@ ComputeCallSiteTable(SmallVectorImpl<CallSiteEntry> &CallSites,
> > }
> >
> > // If some instruction between the previous try-range and the end of the
> > - // function may throw, create a call-site entry with no landing pad for the
> > - // region following the try-range.
> > - if (SawPotentiallyThrowing && Asm->MAI->isExceptionHandlingDwarf()) {
> > + // function requires an unwind table entry, create a call-site entry with no
> > + // landing pad for the region following the try-range.
> > + if (ForceCallSiteEntry && Asm->MAI->isExceptionHandlingDwarf()) {
> > CallSiteEntry Site = { LastLabel, 0, 0, 0 };
> > CallSites.push_back(Site);
> > }
> > diff --git a/lib/CodeGen/AsmPrinter/DwarfException.h b/lib/CodeGen/AsmPrinter/DwarfException.h
> > index a28eaf0..ba02f70 100644
> > --- a/lib/CodeGen/AsmPrinter/DwarfException.h
> > +++ b/lib/CodeGen/AsmPrinter/DwarfException.h
> > @@ -85,9 +85,9 @@ protected:
> > SmallVectorImpl<ActionEntry> &Actions,
> > SmallVectorImpl<unsigned> &FirstActions);
> >
> > - /// CallToNoUnwindFunction - Return `true' if this is a call to a function
> > - /// marked `nounwind'. Return `false' otherwise.
> > - bool CallToNoUnwindFunction(const MachineInstr *MI);
> > + /// CallNeedsUnwindTableEntry - Return `true' if this is a call to a function
> > + /// that needs an unwind table entry. Return `false' otherwise.
> > + bool CallNeedsUnwindTableEntry(const MachineInstr *MI);
> >
> > /// ComputeCallSiteTable - Compute the call-site table. The entry for an
> > /// invoke has a try-range containing the call, a non-zero landing pad and an
> > diff --git a/test/CodeGen/X86/gcc_except_table.ll b/test/CodeGen/X86/gcc_except_table.ll
> > index fcc4e9f..b1b8490 100644
> > --- a/test/CodeGen/X86/gcc_except_table.ll
> > +++ b/test/CodeGen/X86/gcc_except_table.ll
> > @@ -24,10 +24,75 @@ eh.resume:
> > resume { i8*, i32 } %0
> > }
> >
> > -declare void @_Z1fv() optsize
> > -
> > -declare i32 @__gxx_personality_v0(...)
> > -
> > ; CHECK: Leh_func_end0:
> > ; CHECK: GCC_except_table0
> > ; CHECK: = Leh_func_end0-
> > +
> > +define void @test_uwtable() uwtable {
> > + call void @uwtable()
> > + invoke void @_Z1fv() to label %ret unwind label %unwind
> > +
> > +ret:
> > + ret void
> > +
> > +unwind:
> > + %1 = landingpad { i8*, i32 } personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*)
> > + cleanup
> > + resume { i8*, i32 } %1
> > +}
> > +
> > +; CHECK: GCC_except_table1
> > +; CHECK: Call Site 1
> > +; CHECK: has no landing pad
> > +; CHECK: Call Site 2
> > +; CHECK: jumps to
> > +; CHECK: Call Site 3
> > +; CHECK: has no landing pad
> > +
> > +define void @test_nounwind() uwtable {
> > + call void @nounwind()
> > + invoke void @_Z1fv() to label %ret unwind label %unwind
> > +
> > +ret:
> > + ret void
> > +
> > +unwind:
> > + %1 = landingpad { i8*, i32 } personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*)
> > + cleanup
> > + resume { i8*, i32 } %1
> > +}
> > +
> > +; CHECK: GCC_except_table2
> > +; CHECK: Call Site 1
> > +; CHECK: jumps to
> > +; CHECK: Call Site 2
> > +; CHECK: has no landing pad
> > +
> > +define void @test_both() uwtable {
> > + call void @both()
> > + invoke void @_Z1fv() to label %ret unwind label %unwind
> > +
> > +ret:
> > + ret void
> > +
> > +unwind:
> > + %1 = landingpad { i8*, i32 } personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*)
> > + cleanup
> > + resume { i8*, i32 } %1
> > +}
> > +
> > +; CHECK: GCC_except_table3
> > +; CHECK: Call Site 1
> > +; CHECK: has no landing pad
> > +; CHECK: Call Site 2
> > +; CHECK: jumps to
> > +; CHECK: Call Site 3
> > +; CHECK: has no landing pad
> > +
> > +declare void @uwtable() uwtable;
> > +declare void @nounwind() nounwind;
> > +declare void @both() uwtable nounwind;
> > +
> > +declare void @_Z1fv() optsize
> > +
> > +declare i32 @__gxx_personality_v0(...)
> > --
> > 1.9.0.rc3
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list