Page MenuHomePhabricator

[asan-assembly-instrumentation] Added CFI directives to the generated instrumentation code.
ClosedPublic

Authored by ygorshenin on Sep 4 2014, 5:20 AM.

Details

Summary

[asan-assembly-instrumentation] Added CFI directives to the generated instrumentation code.

Diff Detail

Repository
rL LLVM

Event Timeline

ygorshenin updated this revision to Diff 13250.Sep 4 2014, 5:20 AM
ygorshenin retitled this revision from to [asan-assembly-instrumentation] Added CFI directives to the generated instrumentation code..
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).
eugenis edited edge metadata.Sep 4 2014, 5:31 AM

Tests please.

eugenis added inline comments.Sep 4 2014, 5:44 AM
include/llvm/MC/MCTargetAsmParser.h
126 ↗(On Diff #13250)

Please add a comment.
How does it behave when parsing top-level asm?

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
316 ↗(On Diff #13250)

Don't forget end-to-end tests for both cases (esp and non-esp), i.e. omit-frame-pointer and no-omit-frame-pointer.

557 ↗(On Diff #13250)

Is it the right way to obtain MCRegisterInfo?
Direct calls to TheXXXTarget seem to be rather uncommon.

ygorshenin updated this revision to Diff 13446.Sep 9 2014, 3:07 AM
ygorshenin edited edge metadata.

Fixes.

PTAL

End-to-end test is added in http://reviews.llvm.org/D5260.

include/llvm/MC/MCTargetAsmParser.h
126 ↗(On Diff #13250)

Done.

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
557 ↗(On Diff #13250)

It seems that it's possible to get MCRegisterInfo from MCContext, thanks!

eugenis added inline comments.Sep 9 2014, 3:51 AM
include/llvm/MC/MCTargetAsmParser.h
127 ↗(On Diff #13446)

It's better to document what the function does (or does not do), rather then when it is called.
For example, it looks like FrameReg stays uninitialized if it is not called, resulting in undefined behavior for top-level asm.

test/Instrumentation/AddressSanitizer/X86/asm_cfi.ll
45 ↗(On Diff #13446)

attribute 1 and 3 seem to be unused
maybe remove all the fp-math stuff from 2 and 4, too

49 ↗(On Diff #13446)

remove this, and all !srcloc.

PTAL

include/llvm/MC/MCTargetAsmParser.h
127 ↗(On Diff #13446)

FrameReg is a field in X86AsmInstrumentation and initialized to X86::NoRegister in the ctor.

test/Instrumentation/AddressSanitizer/X86/asm_cfi.ll
45 ↗(On Diff #13446)

Done.

49 ↗(On Diff #13446)

Done.

eugenis accepted this revision.Sep 9 2014, 5:21 AM
eugenis edited edge metadata.

LGTM

test/Instrumentation/AddressSanitizer/X86/asm_cfi.ll
45 ↗(On Diff #13449)

Please remove "stack-protector-buffer-size"="8", too.

This revision is now accepted and ready to land.Sep 9 2014, 5:21 AM
ygorshenin updated this revision to Diff 13456.Sep 9 2014, 6:11 AM
ygorshenin edited edge metadata.

Fixes.

Many thanks!

test/Instrumentation/AddressSanitizer/X86/asm_cfi.ll
45 ↗(On Diff #13449)

Done.

ygorshenin closed this revision.Sep 10 2014, 2:55 AM
ygorshenin updated this revision to Diff 13522.

Closed by commit rL217482 (authored by @ygorshenin).