Thanks for fixing it so quickly.
If they reply soon please let us know (probably best to add a comment to
this test).

Hi Julian,

Hi @hctim, thanks for the patch.
I have one question, though. Do you really need to remove the information you removed?
Some people might be testing ASan binaries without access to debug info (because it's been stripped or it's not been loaded and is not available for the sanitizers to get symbols from, etc), and having the full global information would be useful for those reports.

Looks ok, I’ll check it out when I can and submit a patch if needed.

Please make sure to test on Windows (or check if some windows buildbotbis
running those tests and check it out). Shlex is, IIRC, using UNIX rules to
do the split.

Yeah. I looked at it again and looks ok. I missed that we were already
including the file. As long as we can support lsan-less asan, we’re cool.

Please don’t do this. You’re adding a dependency between asan and lsan, and
making it impossible to support platforms without lsan.

I would love to have a single source of truth, but this basically makes it
impossible to just build compiler-rt by itself, no?

Hi Vitaly,

LGTM, please check if adding the test I request makes sense.

It seems there's a FIXME anticipating this problem.

In D56485#1384700, @marxin wrote:

In D56485#1383378, @dexonsmith wrote:

In D56485#1383077, @marxin wrote:

Hi, there's no reply from the people who were added to this review.
May I understand that as ready to be installed?

filcab is still marked as blocking this. Also I thought you were going to file a new differential that had the commits list cc’ed correctly? See above where I asked you to add a couple of relevant reviewers to the new differential (and me as a subscriber) when you do.

@filcab : Are you fine with the patch as it is right now?

@dexonsmith: I'm not sure why a new differential should be created? I updated reviewers in this one.

In D57224#1371302, @mgorny wrote:

if you are concerned about dependencies in non-standalone builds, they are handled separately from this. This addition was only done for COMPILER_RT_STANDALONE_BUILD.

Hah, I had the exact same fix on our private branch.

In D56485#1364241, @pcc wrote:

This change would only impact the "diagnostic" (non-production) mode of CFI. In production builds we do not link the runtime library at all. So from a CFI perspective this is fine.

On the platform I most care about, we have defined SANITIZER_NON_UNIQUE_TYPEINFO, so I am ok with this patch from our point of view. But I want to make sure other platforms using this are ok with the additional slowdown (assuming they have typeinfo equality).

I have a few problems/questions with this patch as it is right now. Please address them before committing.

Hi Eric,

Sorry to ressurect this review, but I have a few questions:

• What kind of functions fail?
• Are there bugzillas to track these?
• How can a compiler expect to have blacklists for "all" its CFI clients? (I'm ok with having a default for "most-used, well-known, problematic functions", e.g: functions in libstdc++ or libc++)
• clang/compiler-rt don't seem to have a default blacklist, what should the contents be? This patch seems to be saying "There are some well-known functions that should never be instrumented with CFI, I'll provide a list of names", which doesn't really seem possible to do in general, for "most" CFI-toolchain clients. As I see it, each client will need to have their own blacklist, and provide it as an argument if needed. Which brings us to:
• If I pass -fsanitize-blacklist=my_blacklist.txt I still get an error. This is not ideal, as I might be working on the blacklist to include in my toolchain/program.

Thanks for the review.
Unfortunately, I forgot to edit the commit message. Let's hope no one gets too confused (phab reviews will be up anyway, so should be easy to figure out).
Filipe

Reworded with feedback from the review.

Expand yet a bit more.

Expanded a bit more on the documentation, mentioning that user code might be technically allowed to access those array cookies, but that users might not want to allow it.

Why is this failing?

LGTMing.

In D53869#1280716, @lebedev.ri wrote:

Feel like actually marking these two reviews as accepted? :)

Looks good, thank you. Ideally, we'd have different names on those functions, but unless you have a script that generates this and is very easy to change, I'm ok with keeping them as they are.

I have minor questions in inline comments, but sorry, I'll request a big change :-/

In D52615#1278000, @rjmccall wrote:

So, three points:

• That's not class-member-specific; you can have a placement operator new[] at global scope that isn't the special void* placement operator and therefore still has a cookie, and it would have just as much flexibility as a class-member override would. So the split you're trying to describe is the standard operators vs. custom ones.

• Anyone can provide their own definition of the standard operators; there are some semantic restrictions on those definitions, but I'm not sure what about those restrictions would forbid this kind of capture.

• Even with the standard implementations of the standard replaceable operators, I'm not sure what rule would actually outlaw the client from going from the result of new[] back to the cookie.

At any rate, taking the feature as a given, the first point suggests -fsanitize-address-poison-custom-array-cookie or something along those lines. If we want a more standard-ese term than "custom", the standard refers to its operators collectively as "library allocation functions", so maybe "non-library".

Missed the change in some places

Update with name change, using rjmccall's suggestion

In D52615#1276938, @rjmccall wrote:

In D52615#1275946, @filcab wrote:

In D52615#1272725, @rjmccall wrote:

In D52615#1266320, @filcab wrote:

In D52615#1254567, @rsmith wrote:

This seems like an unreasonably long flag name. Can you try to find a shorter name for it?

-fsanitize-poison-extra-operator-new?
Not as explicit, but maybe ok if documented?

-fsanitize-address-poison-array-cookie?

I strongly dislike this one because "poison array cookie", in general, is always enabled (it's actually triggered by a runtime flag). This flag is about poisoning it in more cases (cases where the standard doesn't completely disallow accesses to the cookie, so we have to have a flag and can't enable it all the time).

Hmm. This naming discussion might be a proxy for another debate. I understand the argument that programs are allowed to assume a particular C++ ABI and therefore access the array cookie. And I can understand having an option that makes ASan stricter and disallow accessing the array cookie anyway. But I don't understand why this analysis is in any way case-specific, and the discussion you've linked doesn't seem particularly clarifying about that point. Can you explain this a little?

In D52615#1272725, @rjmccall wrote:

In D52615#1266320, @filcab wrote:

In D52615#1254567, @rsmith wrote:

This seems like an unreasonably long flag name. Can you try to find a shorter name for it?

-fsanitize-poison-extra-operator-new?
Not as explicit, but maybe ok if documented?

-fsanitize-address-poison-array-cookie?

Thank you,
Filipe

In D52615#1254567, @rsmith wrote:

This seems like an unreasonably long flag name. Can you try to find a shorter name for it?

LGTM on the clang side too.

Very sorry for the delay, and especially sorry for asking about the same thing over and over :|
Thanks for your patience,

Sorry about that. I’m away today but I don’t think you’ve answered my
questions about “why not support standalone UBSan in tests”. Sorry if I
missed the answers if you did. Will review the latest tomorrow.

0 only means “one of those two”, so I prefer your current patch then a
change to genericUB.

Thanks for working on this,
Filipe

LGTM. Thank you!

In D51648#1224846, @delcypher wrote:

For the iOS simulator the host and the device file system are the same so no work is required. For iOS devices the scripts handle this by copying back the log file to the host when they detect log_path= was in ASAN_OPTIONS/TSAN_OPTIONS/UBSAN_OPTIONS.

This made me curious: Why don't the scripts delete the file after copying it to the host?
If the problem here is the extra files leftover, the scripts for iOS devices can handle it, as they already know which files need to be copied. Then we can even avoid adding anything here. It's a much more contained change.

In D51648#1225124, @george.karpenkov wrote:

To run just-built programs for tests. Not to run anything else

What? Why?
How else can you run commands on the device (cat/cp/rm/etc) and why the semantics of those is different?
Many sanitizer tests already need those commands (e.g. this was recently done for libFuzzer to support device integration)

Most lit commands are run on the host. The %run substitution is there for exactly that: "Take this just-built target executable and run it with these args".
All the cat/rm/etc commands are run on the host, as expected.

In D51648#1225001, @george.karpenkov wrote:

@filcab The script is in iossim folder, so it's specifically for iOS simulators. Arbitrary devices and their support is irrelevant for this change.

In D51648#1224846, @delcypher wrote:

In D51648#1224618, @filcab wrote:

I don't think this is acceptable.
We have no guarantees we even have a shell on the devices. The run script might be doing all sort of things for commands to run on the device.

That is true. However for all scripts that are in-tree where this test is meant to pass (currently only the iOS simulator) I've fixed them to work so that %run rm has a special meaning.
We have out-of-tree scripts for running on iOS devices and I've modified those to also handle the %run rm semantics that is being proposed in this patch.

Out of tree consumers of compiler-rt that have their own device test infrastructure need (if this patch is merged) to specially handle %run rm.

%run has been used for one thing: To run just-built programs for tests. Not to run anything else. I don't think we should start to add more functionality to it, especially if we're just be going to special-case commands on it. We can just as easily create different substitutions. I see %run as having a very precise meaning, and not as a "this is a shell-like entry point to the device".

I don't think this is acceptable.
We have no guarantees we even have a shell on the devices. The run script might be doing all sort of things for commands to run on the device.

I have a nit I'd like to see addressed, otherwise LGTM.

Phabricator didn't pick it up. This was committed on July 10.

Thank you,
Filipe

In D48959#1153246, @lebedev.ri wrote:

Like i said, not ready for the review :)

Looks good. I have a few questions, and would like some additional tests (explained in the comments).

Commandeering revision and posting an updated patch. Please review.

"Commandeering" revision. I will post an updated patch in a bit, if all goes well with arc.

Check out test/sanitizer_common/TestCases/options-include.cc as an example you can use to adapt yours.
You might only need to change env_asan_opts to env_tool_opts to get it to work.

Why create yet another way to disable the printing of warnings?
Why not use the suppressions mechanism which is already there?

We end up not needing this with our current setup, so no need to change it.
Thank you,
Filipe

In D48142#1142349, @filcab wrote:

Clang still warns:

/Users/filcab/dev/llvm/compiler-rt/lib/asan/asan_allocator.cc:793:18: warning: declaration requires a global constructor [-Wglobal-constructors]
static Allocator instance(LINKER_INITIALIZED);
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

Clang still warns:

This got committed, but clang still warns: