- User Since
- Jan 10 2013, 12:14 PM (401 w, 2 d)
Sun, Sep 13
I would love to have a single source of truth, but this basically makes it
impossible to just build compiler-rt by itself, no?
Thu, Sep 3
Mar 10 2020
LGTM, please check if adding the test I request makes sense.
Feb 4 2020
Oct 7 2019
It seems there's a FIXME anticipating this problem.
Sep 2 2019
Feb 5 2019
Jan 25 2019
Hah, I had the exact same fix on our private branch.
Jan 21 2019
Jan 18 2019
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).
Jan 17 2019
I have a few problems/questions with this patch as it is right now. Please address them before committing.
Dec 3 2018
Nov 8 2018
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.
Nov 2 2018
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).
Reworded with feedback from the review.
Nov 1 2018
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.
Oct 30 2018
Why is this failing?
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 :-/
Missed the change in some places
Update with name change, using rjmccall's suggestion
Oct 26 2018
Oct 25 2018
Oct 23 2018
Oct 16 2018
Oct 10 2018
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,
Oct 8 2018
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.
Sep 29 2018
0 only means “one of those two”, so I prefer your current patch then a
change to genericUB.
Thanks for working on this,
Sep 27 2018
Sep 13 2018
LGTM. Thank you!
Sep 5 2018
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.
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.
%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.
Aug 2 2018
Aug 1 2018
Jul 26 2018
I have a nit I'd like to see addressed, otherwise LGTM.
Jul 20 2018
Phabricator didn't pick it up. This was committed on July 10.
Jul 16 2018
Jul 12 2018
Jul 10 2018
Jul 6 2018
Jul 5 2018
Looks good. I have a few questions, and would like some additional tests (explained in the comments).
Jul 4 2018
Jul 3 2018
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.
Jun 29 2018
Why create yet another way to disable the printing of warnings?
Why not use the suppressions mechanism which is already there?
Jun 28 2018
Jun 26 2018
We end up not needing this with our current setup, so no need to change it.
Jun 25 2018
Clang still warns:
This got committed, but clang still warns:
Jun 15 2018
Jun 8 2018
Jun 6 2018
I have a minor nit + what Paul mentioned (missing a -NOT check). Otherwise LGTM.