This is an archive of the discontinued LLVM Phabricator instance.

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

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

Diff Detail

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
289

What about MCStreamer::EmitCFIDefCfa?

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
586

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

857

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

875

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
2

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

ygorshenin edited edge metadata.

Fixes.

PTAL

lib/MC/MCStreamer.cpp
289

Done.

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
586

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.

857

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

875

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
2

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
586

Yes, please rename.

875

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
586

Done.

857
875

Done.

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

lib/MC/MCStreamer.cpp
223

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

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

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).