Page MenuHomePhabricator

[asan-asm-instrumentation] CFI directives are generated for .S files.
ClosedPublic

Authored by ygorshenin on Sep 29 2014, 7:03 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ygorshenin updated this revision to Diff 14163.Sep 29 2014, 7:03 AM
ygorshenin updated this revision to Diff 14164.
ygorshenin retitled this revision from to [asan-asm-instrumentation] CFI directives are generated for .S files..
ygorshenin updated this object.
ygorshenin edited the test plan for this revision. (Show Details)
ygorshenin added a reviewer: eugenis.
ygorshenin added a subscriber: Unknown Object (MLST).

asm_cfi.s file is added to git index.

eugenis edited edge metadata.Sep 30 2014, 12:51 AM

Please don't forget about a compiler-rt test (either in this or in a separate commit).

lib/MC/MCStreamer.cpp
278 ↗(On Diff #14164)

What about MCStreamer::EmitCFIDefCfa?

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
579 ↗(On Diff #14164)

This is shadowing X86AsmInstrumentation::FrameReg. Confusing.
Perhaps rename the other one to smth like "InitialFrameReg"?

850 ↗(On Diff #14164)

Please check (and test) that all this stuff works with -fno-unwind-tables.

868 ↗(On Diff #14164)

Is it even possible? Can we CHECK instead?
Is it the same situation as "No active dwarf frame" above?

test/Instrumentation/AddressSanitizer/X86/asm_cfi.s
1 ↗(On Diff #14164)

Please add at least a one-line comment on what this test is about.

ygorshenin edited edge metadata.

Fixes.

PTAL

lib/MC/MCStreamer.cpp
278 ↗(On Diff #14164)

Done.

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
579 ↗(On Diff #14164)

That's why I moved FrameReg to the private part of the X86AsmInstrumentation and there're no shadowing. But if you still insisting I'll rename X86AsmInstrumentation::FrameReg to something like InitialFrameReg.

850 ↗(On Diff #14164)

Sure, but could you please explain what -fno-unwind-tables does?

868 ↗(On Diff #14164)

MCDwarfFrameInfo::CfaRegister is set when something like .cfi_def_cfa_register was emitted. Otherwise it won't be set (even between .cfi_startproc and .cfi_endproc).

test/Instrumentation/AddressSanitizer/X86/asm_cfi.s
1 ↗(On Diff #14164)

Done.

D5547 is good, but I'd also like a line in asm_cfi.ll test that checks that we don't add any CFI instructions if there is no .cfi_startproc (i.e. unwind tables are off).

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
579 ↗(On Diff #14164)

Yes, please rename.

868 ↗(On Diff #14164)

And this makes MCDwarfFrameInfo::CfaRegister unreliable, which is not so good.
There are target-dependent initial CFI instructions, see MCAsmInfo::addInitialFrameState. Could you make CfaRegister take them into account?

PTAL

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
579 ↗(On Diff #14164)

Done.

850 ↗(On Diff #14164)
868 ↗(On Diff #14164)

Done.

LGTM, but I'd like someone else to vet the changes to MCStreamer / MCDwarf.

lib/MC/MCStreamer.cpp
223 ↗(On Diff #14273)

I don't really like that we do this extra work (a) for each function and (b) even when there is no instrumentation and CurrentCfaRegister is unused. But all alternatives seem much more complex, and this loop typically has just one or two iterations, so maybe it's not worth optimizing.

Rafael, could you please take a look?

rafael edited edge metadata.Oct 6 2014, 9:34 PM

LGTM with a nit.

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
868 ↗(On Diff #14273)

The X86::NoRegister in here is the 0 in CurrentCfaRegister(0), no? I would probably be cleaner no just use 0 everywhere.

This could also be an assert now, no? The initial instructions should always be setting the initial frame register.

rafael accepted this revision.Oct 6 2014, 9:34 PM
rafael edited edge metadata.
This revision is now accepted and ready to land.Oct 6 2014, 9:34 PM
ygorshenin updated this revision to Diff 14499.Oct 7 2014, 4:10 AM
ygorshenin edited edge metadata.

Fixes.

Many thanks, Eugene and Rafael!

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
868 ↗(On Diff #14273)

Done.

ygorshenin closed this revision.Oct 7 2014, 4:13 AM
ygorshenin updated this revision to Diff 14500.

Closed by commit rL219199 (authored by @ygorshenin).