This is an archive of the discontinued LLVM Phabricator instance.

[sanitizers] Disable target-specific lowering of string functions.
ClosedPublic

Authored by koriakin on May 1 2016, 6:26 PM.

Details

Summary

CodeGen has hooks that allow targets to emit specialized code instead
of calls to memcmp, memchr, strcpy, stpcpy, strcmp, strlen, strnlen.
When ASan/MSan/TSan/ESan is in use, this sidesteps its interceptors, resulting
in uninstrumented memory accesses. To avoid that, make these sanitizers
mark the calls as nobuiltin.

Depends on D19801.

Diff Detail

Repository
rL LLVM

Event Timeline

koriakin retitled this revision from to [ASan] Disable target-specific lowering of string functions..
koriakin updated this object.
koriakin added reviewers: kcc, eugenis, resistor.
koriakin set the repository for this revision to rL LLVM.
koriakin added a project: Restricted Project.
koriakin added a subscriber: llvm-commits.

Seems like a good idea. Is there a place you could hoist the no isNoBuiltin checks?

Now the nobuiltin check applies to all libcalls (which makes sense, after all).

filcab added a subscriber: filcab.May 2 2016, 4:15 AM

Thanks for working on this.
Can you split this patch in two, please?
The SelectionDAG + CodeGen tests on one, the ASan-specific code on another.
Not having a proper way to test SelectionDAG is annoying. To have perfecct coverage, we'd need to replicate those tests for other targets. This will do for now, though. :-)

BTW, have you measured performance impact?
If it's too big, we probably want to put this under a flag (the sanitizer philosophy is that we know we have false negatives, but no false positives, so it fits not doing this by default if it's a big performance hit).

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
6177 ↗(On Diff #55799)

We probably want to check for isNoBuiltin before we query LibInfo, which involves lookups.

lib/Transforms/Instrumentation/AddressSanitizer.cpp
1859 ↗(On Diff #55799)

We probably want to add some of the other intercepted functions (but instead of duplicating everything, I guess it's better to only add those that actually have special handling).
That can be a follow up patch, though, as needed.

Thanks for working on this.
Can you split this patch in two, please?
The SelectionDAG + CodeGen tests on one, the ASan-specific code on another.
Not having a proper way to test SelectionDAG is annoying. To have perfecct coverage, we'd need to replicate those tests for other targets. This will do for now, though. :-)

Alright, I'll split it up. However, there's not much to test for other platforms - only SystemZ currently has customised lowering for those functions.

BTW, have you measured performance impact?
If it's too big, we probably want to put this under a flag (the sanitizer philosophy is that we know we have false negatives, but no false positives, so it fits not doing this by default if it's a big performance hit).

I haven't - I'm still trying to get the testsuite passing on SystemZ. However, all other platforms are perfectly fine with always running the interceptors, so I doubt there's anything to gain here.

filcab accepted this revision.May 2 2016, 4:57 AM
filcab added a reviewer: filcab.

Since no-one else does anything special with the libcalls for now, I'm totally ok with the patch.
LGTM with the change + split I asked, then. But wait to see if Eric has more comments.

This revision is now accepted and ready to land.May 2 2016, 4:57 AM
filcab added inline comments.May 2 2016, 5:28 AM
test/Instrumentation/AddressSanitizer/str-nobuiltin.ll
3 ↗(On Diff #55803)

Do we need -asan-module? We should be able to get away with just the FunctionPass, right?

koriakin edited edge metadata.

Removed -asan-module

eugenis edited edge metadata.May 2 2016, 1:29 PM

Do we want this in the other sanitizers? Probably yes, and with the same list of functions.

lib/Transforms/Instrumentation/AddressSanitizer.cpp
1785 ↗(On Diff #55807)

We typically write single-statement ifs without the braces.

1844 ↗(On Diff #55807)

Did you mean maybeMarkCallNoBuiltin ?

koriakin marked an inline comment as done.May 2 2016, 1:40 PM

Do we want this in the other sanitizers? Probably yes, and with the same list of functions.

Yeah, I think so. What would be a good place to put the maybeMark...() function?

koriakin planned changes to this revision.May 2 2016, 1:54 PM

lib/Transforms/Utils/ModuleUtils.cpp, or maybe a new file in that directory.

koriakin retitled this revision from [ASan] Disable target-specific lowering of string functions. to [sanitizers] Disable target-specific lowering of string functions..
koriakin updated this object.
koriakin edited edge metadata.
koriakin marked 2 inline comments as done.

Added MSan and TSan to the mix. LSan and UBSan don't intercept these, DFSan already mangles all symbols and avoids the issue, and I have no clue about ESan.

This revision is now accepted and ready to land.May 3 2016, 5:47 AM
koriakin updated this object.

Added ESan as well.

eugenis added inline comments.May 3 2016, 12:58 PM
include/llvm/Transforms/Utils/Local.h
358 ↗(On Diff #56004)

I don't think this is a good name for a global function. It is specific to the sanitizer interceptors. How about maybeMarkSanitizerLibraryCallNoBuiltin?

Rather wordy (I'm having problems wrapping the lines), but it's indeed better.

eugenis accepted this revision.May 3 2016, 2:12 PM
eugenis edited edge metadata.

LGTM

This revision was automatically updated to reflect the committed changes.