nlopes (Nuno Lopes)
User

Projects

User does not belong to any projects.

User Details

User Since
Oct 3 2013, 11:31 AM (215 w, 3 d)

Recent Activity

Thu, Nov 9

nlopes committed rL317815: revert r317812 [BasicAA] fix build break by converting the previously….
revert r317812 [BasicAA] fix build break by converting the previously…
Thu, Nov 9, 9:35 AM
nlopes committed rL317812: [BasicAA] fix build break by converting the previously introduced assert into….
[BasicAA] fix build break by converting the previously introduced assert into…
Thu, Nov 9, 9:06 AM
nlopes committed rL317803: [BasicAA] add assertion for corner case in aliasGEP().
[BasicAA] add assertion for corner case in aliasGEP()
Thu, Nov 9, 8:17 AM

Wed, Nov 8

nlopes committed rL317680: BasicAA: fix bug where we would return partialalias instead of noalias.
BasicAA: fix bug where we would return partialalias instead of noalias
Wed, Nov 8, 2:59 AM

Sat, Oct 28

nlopes committed rL316838: [pubs.js] add poison paper.
[pubs.js] add poison paper
Sat, Oct 28, 12:30 PM

Sep 21 2017

nlopes added a comment to D37832: Eliminate PHI (int typed) which is only used by inttoptr.

Ok, mea culpa, I thought CreateBitOrPointerCast() would simply create a bitcast.
Then, what we have here is:

v = phi(ptr2int(p), ptr2int(q))
 =>
v = ptr2int(phi(p, q))
Sep 21 2017, 8:35 AM

Sep 20 2017

nlopes added a comment to D37832: Eliminate PHI (int typed) which is only used by inttoptr.

As I have mentioned, this patch itself does *not* fold any ptrtoint/inttoptr. It simply moves the intptr across the phi node. The folding you see with the test cases is done by existing optimizations, so I am not sure what the objection is about.

Sep 20 2017, 3:56 AM

Sep 19 2017

nlopes added a comment to D37832: Eliminate PHI (int typed) which is only used by inttoptr.

Ok, so after another scan through the patch and a discussion with Gil, I must say the transformation is not fully correct.

Sep 19 2017, 6:24 AM

Sep 18 2017

nlopes added a comment to D37832: Eliminate PHI (int typed) which is only used by inttoptr.

This transformation looks correct to me and goes in the right direction (remove unneeded int2ptr/ptr2int, since they block many optimizations). The patch can be made a bit more general later.
Before the patch goes in, please add a few negative tests; you only have positive ones. Thanks!

Sep 18 2017, 10:30 AM
nlopes added a comment to D37832: Eliminate PHI (int typed) which is only used by inttoptr.

This transformation looks correct to me and goes in the right direction (remove unneeded int2ptr/ptr2int, since they block many optimizations). The patch can be made a bit more general later.
Before the patch goes in, please add a few negative tests; you only have positive ones. Thanks!

Sep 18 2017, 6:17 AM

Sep 13 2017

nlopes accepted D37713: [InstSimplify] fold sdiv/srem based on compare of dividend and divisor.

LGTM, thanks.
You're right: the Boolean connector for the second transformation was flipped. All good now :)

Sep 13 2017, 11:26 AM

Sep 12 2017

nlopes requested changes to D37713: [InstSimplify] fold sdiv/srem based on compare of dividend and divisor.

I was trying to prove this in Alive, but the proof doesn't go through.
Some corner cases are not correct: http://rise4fun.com/Alive/iVs
For example: 'INT_MIN / something' would be replaced with 0, but shouldn't.

Sep 12 2017, 2:50 PM

Sep 9 2017

nlopes committed rL312870: clang fix for LLVM API change: isKnownNonNull -> isKnownNonZero.
clang fix for LLVM API change: isKnownNonNull -> isKnownNonZero
Sep 9 2017, 11:27 AM
nlopes committed rL312869: Merge isKnownNonNull into isKnownNonZero.
Merge isKnownNonNull into isKnownNonZero
Sep 9 2017, 11:25 AM
nlopes closed D37628: Cleanup: Merge isKnownNonNull into isKnownNonZero by committing rL312869: Merge isKnownNonNull into isKnownNonZero.
Sep 9 2017, 11:25 AM

Sep 8 2017

nlopes created D37628: Cleanup: Merge isKnownNonNull into isKnownNonZero.
Sep 8 2017, 9:09 AM

Sep 6 2017

nlopes committed rL312648: Fix PR33878: BasicAA incorrectly assumes different address spaces don't alias.
Fix PR33878: BasicAA incorrectly assumes different address spaces don't alias
Sep 6 2017, 9:57 AM
nlopes closed D37518: Fix PR33878: BasicAA incorrectly assumes different address spaces don't alias by committing rL312648: Fix PR33878: BasicAA incorrectly assumes different address spaces don't alias.
Sep 6 2017, 9:56 AM
nlopes created D37518: Fix PR33878: BasicAA incorrectly assumes different address spaces don't alias.
Sep 6 2017, 7:41 AM

Aug 12 2017

nlopes added a comment to D36592: [BDCE] clear poison generators after turning a value into zero (PR33695, PR34037).

LGTM, with minor comments: missing the CHECK comment above, and missing a test for the assume removal.

Aug 12 2017, 2:54 AM

Aug 10 2017

nlopes added inline comments to D29011: [IR] Add Freeze instruction.
Aug 10 2017, 3:51 AM

Aug 9 2017

nlopes committed rL310495: CFLAA: return MustAlias when pointers p, q are equal, i.e.,.
CFLAA: return MustAlias when pointers p, q are equal, i.e.,
Aug 9 2017, 10:05 AM

Aug 8 2017

nlopes committed rL310420: BasicAA: assert on another case where aliasGEP shouldn't get a PartialAlias….
BasicAA: assert on another case where aliasGEP shouldn't get a PartialAlias…
Aug 8 2017, 2:26 PM
nlopes committed rL310373: BasicAA: aliasGEP shouldn't get a PartialAlias response here.
BasicAA: aliasGEP shouldn't get a PartialAlias response here
Aug 8 2017, 9:16 AM

Jul 29 2017

nlopes added a comment to D35887: isKnownNonNull: recognize GEP inbounds as non-null.

isKnownNonZero was added in r124183; isKnownNonNull was originally part of BasicAA, then moved in r151446.

Jul 29 2017, 2:31 PM

Jul 28 2017

nlopes abandoned D35887: isKnownNonNull: recognize GEP inbounds as non-null.
Jul 28 2017, 6:12 AM

Jul 27 2017

nlopes added a comment to D35887: isKnownNonNull: recognize GEP inbounds as non-null.

Just realized there's a complicated isGEPKnownNonNull() in ValueTracking.cpp, which is use solely by isKnownNonZero().
I don't believe such a thing is necessary, since the input pointer to a GEP inbounds has to be non-null in address space 0. If others agree I'll also remove that function in a subsequent patch.

Jul 27 2017, 1:58 AM

Jul 26 2017

nlopes committed rL309117: [docs] change a few code-blocks to llvm from text.
[docs] change a few code-blocks to llvm from text
Jul 26 2017, 7:14 AM
nlopes resigned from D7583: asan: do not instrument direct inbounds accesses to stack variables.
Jul 26 2017, 5:58 AM
nlopes updated subscribers of D35887: isKnownNonNull: recognize GEP inbounds as non-null.
Jul 26 2017, 5:55 AM
nlopes created D35887: isKnownNonNull: recognize GEP inbounds as non-null.
Jul 26 2017, 5:55 AM

Jul 25 2017

nlopes accepted D35811: A workaround for the bug caused by descrepancy between loop-unswitch and GVN about branch on undef.

LGTM.
Seems good enough to me as a temporary hack.

Jul 25 2017, 1:05 AM

Jul 15 2017

nlopes committed rL308090: [docs] AliasAnalysis: clarify that PartialAlias doesn't enforce.
[docs] AliasAnalysis: clarify that PartialAlias doesn't enforce
Jul 15 2017, 2:09 AM

Jun 6 2017

nlopes committed rL304780: [docs] Make it clear shifts yield poison when shift amount >= bitwidth.
[docs] Make it clear shifts yield poison when shift amount >= bitwidth
Jun 6 2017, 1:28 AM
nlopes closed D33654: [docs] Make it clear shifts yield poison when shift amount >= bitwidth by committing rL304780: [docs] Make it clear shifts yield poison when shift amount >= bitwidth.
Jun 6 2017, 1:28 AM

May 30 2017

nlopes added a comment to D33654: [docs] Make it clear shifts yield poison when shift amount >= bitwidth.

This one for example (stole from Dave; I don't have my list here handy):
rewrite (1 << Y) * X into X << Y:

May 30 2017, 11:44 AM

May 29 2017

nlopes retitled D33654: [docs] Make it clear shifts yield poison when shift amount >= bitwidth from [docs] Make it clear shifts yield poison when shift ammount >= bitwidth to [docs] Make it clear shifts yield poison when shift amount >= bitwidth.
May 29 2017, 11:59 AM
nlopes created D33654: [docs] Make it clear shifts yield poison when shift amount >= bitwidth.
May 29 2017, 11:59 AM

May 5 2017

nlopes committed rL302246: fix build on Cygwin.
fix build on Cygwin
May 5 2017, 9:21 AM

Mar 27 2017

nlopes added a comment to D20116: Add speculatable function attribute.

Then we have an orthogonal concern which is what's the precondition that is sufficient to justify an optimization. For example, whether a function call can be executed speculatively is one of such preconditions. It can be derived by looking at the function attributes. For example, for speculative execution we probably need to know that the function doesn't write to memory and that it terminates.

It is not enough (for example division by zero).

Mar 27 2017, 1:38 AM

Mar 26 2017

nlopes added a comment to D20116: Add speculatable function attribute.

Let me maybe zoom out and give a different perspective:
Right now call site and function attributes are an AND of predicates that are always guaranteed to hold for that specific call site or for all call sites, respectively.
Predicates include things like doesn't write to memory, only writes to memory that is not observable, etc. Using attributes we can state that several of these predicates hold.
In the ideal world, predicates wouldn't overlap, although since we can only state ANDs of predicates and not ORs, some overlap may be need in practice.

Mar 26 2017, 8:17 AM

Mar 15 2017

nlopes committed rL297816: fix gcc -Wmisleading-indentation [NFC].
fix gcc -Wmisleading-indentation [NFC]
Mar 15 2017, 2:45 AM

Mar 11 2017

nlopes accepted D30834: [x86] these aren't the undefs you're looking for (PR32176).
Mar 11 2017, 1:26 AM

Mar 9 2017

nlopes committed rL297378: fix build on Cygwin.
fix build on Cygwin
Mar 9 2017, 5:55 AM

Mar 8 2017

nlopes added a comment to D30665: [InstSimplify] vector div/rem with any zero element in divisor is undef.

LGTM

Mar 8 2017, 1:38 AM

Feb 12 2017

nlopes added a comment to D29774: [InstCombine] fold icmp sgt/slt (add nsw X, C2), C --> icmp sgt/slt X, (C - C2).

LGTM.
The easy way to check for overflow in Alive: http://rise4fun.com/Alive/MH

Thanks! I should read some docs now. :)
It looks like I should start with the papers and presentations from previous LLVM dev confs...is there also a reference doc for the online tool?

Feb 12 2017, 3:04 PM
nlopes added a comment to D29774: [InstCombine] fold icmp sgt/slt (add nsw X, C2), C --> icmp sgt/slt X, (C - C2).

LGTM.
The easy way to check for overflow in Alive: http://rise4fun.com/Alive/MH

Feb 12 2017, 2:35 AM

Feb 5 2017

nlopes added inline comments to D29519: Add PredicateInfo utility and printing pass.
Feb 5 2017, 8:06 AM

Jan 28 2017

nlopes added a comment to D28952: [analyzer] Add new Z3 constraint manager backend.

Let me give just 2 more Z3-related suggestions:

  • instead of re-creating the solver, it might be faster to do Z3_solver_reset
  • "once in a while" it might be helpful to delete everything (all solvers, asts, context) and call Z3_reset_memory. Z3's small object pool is not very good and keeps growing /ad infinitum/, so cleaning it up once a while is a good thing (improves cache usage and reduces memory consumption).
Jan 28 2017, 12:32 PM

Jan 26 2017

nlopes updated the diff for D29121: [Docs] Add LangRef documention for freeze instruction.
  • rebase
  • add UB semantics to indirectbr and switch
Jan 26 2017, 1:20 AM

Jan 25 2017

nlopes added inline comments to D29014: [SelDag] Implement FREEZE node.
Jan 25 2017, 4:17 AM
nlopes updated subscribers of D29121: [Docs] Add LangRef documention for freeze instruction.
Jan 25 2017, 3:18 AM
nlopes added a dependent revision for D29011: [IR] Add Freeze instruction: D29121: [Docs] Add LangRef documention for freeze instruction.
Jan 25 2017, 3:17 AM
nlopes added a dependency for D29121: [Docs] Add LangRef documention for freeze instruction: D29011: [IR] Add Freeze instruction.
Jan 25 2017, 3:17 AM
nlopes created D29121: [Docs] Add LangRef documention for freeze instruction.
Jan 25 2017, 3:16 AM
nlopes added a comment to D28952: [analyzer] Add new Z3 constraint manager backend.

Regarding incremental solving with Z3 (or with most SMT solvers in general), let me just lower the expectations a bit:
In Z3, when you do push(), there are a few things that change immediately: 1) it uses a different SAT solver (one that supports incremental reasoning), and 2) some preprocessing steps are disabled completely.
The advantage of incremental solving is that it shares the state of the SAT solver between queries. On the other hand, Z3 disables a bunch of nice preprocessors, and also it switches to eager bit-blasting. So whether going incremental pays off, it depends.. I've seen cases where it's faster to create a new solver for each query and cases where incremental is a good thing.
It's on our plans to "fix" Z3 to do better preprocessing in incremental mode, but there's no ETA at the moment to start this work..

Jan 25 2017, 2:18 AM

Jan 24 2017

nlopes accepted D29053: [InstSimplify] try to eliminate icmp Pred (add nsw X, C1), C2.

Alive is still not great for thins kind of patch; I guess it needs some macro support.
But you can use a little more general preconditions like this: http://rise4fun.com/Alive/Tz0

Jan 24 2017, 3:07 AM
nlopes added a dependency for D29016: [LoopUnswitch] Do not freeze condition if hoisted branch is guaranteed to be reachable: D29015: [LoopUnswitch] Fix introduction of UB when hoisted condition may be undef or poison.
Jan 24 2017, 1:14 AM
nlopes added a dependent revision for D29015: [LoopUnswitch] Fix introduction of UB when hoisted condition may be undef or poison: D29016: [LoopUnswitch] Do not freeze condition if hoisted branch is guaranteed to be reachable.
Jan 24 2017, 1:14 AM
nlopes added a dependent revision for D29011: [IR] Add Freeze instruction: D29015: [LoopUnswitch] Fix introduction of UB when hoisted condition may be undef or poison.
Jan 24 2017, 1:14 AM
nlopes added a dependency for D29015: [LoopUnswitch] Fix introduction of UB when hoisted condition may be undef or poison: D29011: [IR] Add Freeze instruction.
Jan 24 2017, 1:14 AM
nlopes added a dependency for D29014: [SelDag] Implement FREEZE node: D29011: [IR] Add Freeze instruction.
Jan 24 2017, 1:13 AM
nlopes added a dependent revision for D29011: [IR] Add Freeze instruction: D29014: [SelDag] Implement FREEZE node.
Jan 24 2017, 1:13 AM
nlopes added a dependency for D29013: Add InstCombine/InstructionSimplify support for Freeze Instruction: D29011: [IR] Add Freeze instruction.
Jan 24 2017, 1:13 AM
nlopes added a dependent revision for D29011: [IR] Add Freeze instruction: D29013: Add InstCombine/InstructionSimplify support for Freeze Instruction.
Jan 24 2017, 1:13 AM
nlopes removed a dependency for D29053: [InstSimplify] try to eliminate icmp Pred (add nsw X, C1), C2: D29011: [IR] Add Freeze instruction.
Jan 24 2017, 1:12 AM
nlopes removed a dependent revision for D29011: [IR] Add Freeze instruction: D29053: [InstSimplify] try to eliminate icmp Pred (add nsw X, C1), C2.
Jan 24 2017, 1:12 AM
nlopes added a dependency for D29053: [InstSimplify] try to eliminate icmp Pred (add nsw X, C1), C2: D29011: [IR] Add Freeze instruction.
Jan 24 2017, 1:12 AM
nlopes added a dependent revision for D29011: [IR] Add Freeze instruction: D29053: [InstSimplify] try to eliminate icmp Pred (add nsw X, C1), C2.
Jan 24 2017, 1:12 AM

Jan 23 2017

nlopes updated the summary of D29016: [LoopUnswitch] Do not freeze condition if hoisted branch is guaranteed to be reachable.
Jan 23 2017, 4:01 AM
nlopes updated the summary of D29015: [LoopUnswitch] Fix introduction of UB when hoisted condition may be undef or poison.
Jan 23 2017, 3:59 AM

Jan 22 2017

nlopes added a comment to D28952: [analyzer] Add new Z3 constraint manager backend.

Cool work Dominic!
Just a few random comments from the peanut gallery regarding the usage of Z3:

  • I would definitely split the Z3Expr into expr/solver/model classes. Several advantages: plugging in new solvers becomes easier and reference counting can be handled more safely and automatically. Right now your solver+model related code handled ref-counting "by hand", which can easily lead to bugs.
  • I would add an rvalue constructor to the expr class to avoid ref-counting when unneeded.
  • Function z3Expr::fromData() seems to be able to create arbitrarily-sized bit-vectors. I would limit to e.g. 512 bits or something like that. Z3 is super slow with huge bit-vectors. If you feel fancy (for a following version of this feature), you could encode this information lazily (based on the model).
  • Z3Expr::fromInt() -- why take a string as input and not a uin64_t?
  • Why call simplify before asserting a constraint? That's usually a pretty slow thing to do (in my experience).
  • You should replace Z3_mk_solver() with something fancier. If you don't feel fancy, just use Z3_mk_simple_solver() (simple is short for simplifying btw). If you know the theory of the constraints (e.g., without floats, it would be QF_BV), you should use Z3_mk_solver_for_theory() (or whatever the name was). If you feel really fancy, you could roll your own tactic (in my tools I often have a tactic class to build my own tactics; Z3 even allows you to case split on the theory after simplifications)
  • Instead of asserting multiple constraints to the solver, I would and() them and assert just once (plays nicer with some preprocessors of Z3)
  • Timeout: SMT solvers sometimes go crazy and take forever. I would advise to add a timeout to the solver to avoid getting caught on a trap. This timeout can be different for the two use cases: check if a path is feasible (lower timeout), or check if you actually have a bug (higher timeout)
  • Also, I didn't see anything regarding memory (I might have missed yet). Are you using the array theory? If so, I would probably advise benchmarking it carefully since Z3's support for arrays is not great (even though it has improved a bit in the last year or so).
Jan 22 2017, 11:48 AM

Oct 6 2016

nlopes committed rL283427: fix build on cygwin.
fix build on cygwin
Oct 6 2016, 6:29 AM

Apr 9 2016

nlopes updated subscribers of D18438: Calculate __builtin_object_size when pointer depends on a condition.
Apr 9 2016, 10:18 AM

Mar 16 2016

nlopes added a comment to D18230: [SimplifyLibCalls] Simplify strlen to a subtraction for certain cases.

Could you please explain the motivation for this transformation?
Doesn't seem very profitable to me unless strlen(s) has already been computed before. Otherwise we end up visiting more memory positions than with the original code.

Mar 16 2016, 4:13 PM

Feb 29 2016

nlopes added a comment to D16986: [LICM] Don't assert on volatile accesses.

"I second my case that we should change TBAA instead, such that it handles
volatile stuff more conservatively."

Volatileness and aliasing are different properties.
It's completely reasonable for TBAA (and other alias analysis) to say two
pointers may or may not access different memory.
It may use any way it feels like to achieve that goal.

The volatileness of a given access is something the optimizer needs to take
into account, not TBAA.

(and yes, you can get completely inconsistent results between different
alias analysis providers, where some will say mustalias and some will say
noalias for the same thing).

TL;DR You should fix LICM or AST, depending on your viewpoint.
You also have to take into account that TBAA does not obey transitiveness
or other various nice properties,not to mention different AA levels giving
inconsistent results.

Only one result is "most correct" at the language semantic level, but both
results are correct at the IR level (ie both noalias and mustalias). It
can use or not use whatever metadata it likes.

(apropos of nothing, the lack of transitivity/etc in TBAA is one of the
reasons effective partitioning schemes for aliasing information don't
usually work out :P. Since AST is basically a partitioning scheme, you are
feeling the pain of the edge cases of TBAA)

Feb 29 2016, 8:41 AM

Feb 28 2016

nlopes added a comment to D16986: [LICM] Don't assert on volatile accesses.

But then store forwarding happens and LICM realizes (or doesn't) that two of the alias sets that were said to be disjoint by TBAA in fact alias.

Can you clarify this point? AFAIK, LICM does not do store forwarding. How did store forwarding come into the picture at all?

Feb 28 2016, 3:12 PM

Feb 22 2016

nlopes added a comment to D16986: [LICM] Don't assert on volatile accesses.

Just a quick comment on this bug. We can see it from two angles:

  1. TBAA is just doing its job since it is exploiting the fact that 2 pointers with different types don't alias (the metadata has 2 different type branches). So TBAA claims the pointers don't alias, while they do in practice (it's undefined behavior, though). -- in this case LICM needs patching to account for this aggressiveness.
  2. Or we can say that TBAA shouldn't mess with volatile stuff since that's dangerous. In that case LICM is fine and TBAA needs patching.

    I'm personally more favorable of 2), since I don't think it's worth to mess with volatile stuff, since it can be dangerous for applications and probably there isn't much performance to gain there.

I disagree with your framing. Unless I misunderstood the issue, this is a clear bug in AST regardless of what TBAA returns for aliasing information. If we ended up with a volatile memory access being part of a alias set, that alias set *must* be marked volatile. It's okay to have non-volatile accesses as part of the volatile set, but not the other way around. If we required the isVolatile flag to be precise for all elements in the set, we'd essentially be requiring alias analysis to *always* get a precise no-alias result when it's possible to do so. That's not an invariant we can uphold.

Feb 22 2016, 3:02 PM

Feb 21 2016

nlopes added a comment to D16986: [LICM] Don't assert on volatile accesses.

Just a quick comment on this bug. We can see it from two angles:

  1. TBAA is just doing its job since it is exploiting the fact that 2 pointers with different types don't alias (the metadata has 2 different type branches). So TBAA claims the pointers don't alias, while they do in practice (it's undefined behavior, though). -- in this case LICM needs patching to account for this aggressiveness.
Feb 21 2016, 11:53 AM

Dec 30 2015

nlopes added a comment to D15820: [MemoryBuiltins] Remove isOperatorNewLike by consolidating non-null inference handling.

LGTM.
I agree with you re InferFunctionAttrs. I don't see any reason why we have InferFunctionAttrs recognize all those library functions instead of having the frontend doing it. But removing it at this point may break someone's code..

Dec 30 2015, 2:25 PM

Dec 16 2015

nlopes added a comment to D14933: Add the allocsize attribute to LLVM.

support arbitrary expressions (like llvm.assume).

I'm fine allowing this attribute to store a Function* or Value* that LLVM tries to constant fold at each callsite, but I can't see how llvm.assume fits into the picture at all. Can you give a concrete example of what you're trying to accomplish with the addition you proposed?

Dec 16 2015, 2:55 PM

Dec 15 2015

nlopes added a comment to D14933: Add the allocsize attribute to LLVM.

I was wondering whether we could go a bit more generic and support arbitrary expressions (like llvm.assume). This design only supports allocation functions that return a pointer to the beginning of a buffer with size p1 * p2 * ... * pn, where pi are the parameters.

Dec 15 2015, 2:17 PM
nlopes updated subscribers of D14933: Add the allocsize attribute to LLVM.
Dec 15 2015, 2:12 PM

Aug 11 2015

nlopes updated subscribers of D11948: Add some macros to abstract marking of parameters as "not null", and use them in <cstring>.
Aug 11 2015, 11:26 AM

Jul 15 2015

nlopes added a comment to D11051: Swap loop invariant GEP with loop variant GEP to allow more LICM..

I don't think GEP reassociation should be done as a canonical InstCombine for two reasons
(1) the inbounds property would need to be dropped

Can you even do this if you don't have inbounds GEPs? Nuno, do you have an opinion about this?

Jul 15 2015, 2:39 AM

Jun 27 2015

nlopes committed rL240880: add 2 PLDI'15 papers.
add 2 PLDI'15 papers
Jun 27 2015, 2:47 AM

May 30 2015

nlopes committed rL238657: ubsan: Check for null pointers given to certain builtins, such.
ubsan: Check for null pointers given to certain builtins, such
May 30 2015, 9:16 AM
nlopes closed D9673: Check for null pointers given to memcpy with ubsan by committing rL238657: ubsan: Check for null pointers given to certain builtins, such.
May 30 2015, 9:16 AM

May 24 2015

nlopes added a comment to D9673: Check for null pointers given to memcpy with ubsan.

Wait a second, won't UBSan handle this automatically if memcpy/memmove are
declared with attribute((nonnull)) in the header? Otherwise, is there
a change to the standard that imposes these additional constraints on
memcpy/memmove?

Not really. memcpy/memmove calls are handled by CGBultin and not CGCall.
It's a different code path.
Nuno

Interesting. I think that if we decide to implement such a check, we shouldn't depend on
attributes specified in the headers, so nonnull-attribute is no longer relevant. There are another
kind of compiler builtins which worth extra checks, and which don't even require headers - e.g. behavior of __builtin_ctz(0)
is undefined. I think we should implement another check kind -fsanitize=builtin that would verify arguments of
various builtin functions.

May 24 2015, 8:57 AM
nlopes added reviewers for D9673: Check for null pointers given to memcpy with ubsan: samsonov, rsmith.
May 24 2015, 8:55 AM

May 11 2015

nlopes added a comment to D9673: Check for null pointers given to memcpy with ubsan.

Wait a second, won't UBSan handle this automatically if memcpy/memmove are
declared with attribute((nonnull)) in the header? Otherwise, is there
a change to the standard that imposes these additional constraints on
memcpy/memmove?

May 11 2015, 2:42 PM
nlopes retitled D9673: Check for null pointers given to memcpy with ubsan from Check for null pointers in ubsan to Check for null pointers given to memcpy with ubsan.
May 11 2015, 12:01 PM
nlopes changed the edit policy for D9673: Check for null pointers given to memcpy with ubsan.
May 11 2015, 12:01 PM
nlopes retitled D9673: Check for null pointers given to memcpy with ubsan from to Check for null pointers in ubsan.
May 11 2015, 12:00 PM

Mar 14 2015

nlopes accepted D8193: asan: fix overflows in isSafeAccess.

LGTM.

Mar 14 2015, 11:32 AM

Feb 25 2015

nlopes requested changes to D7583: asan: do not instrument direct inbounds accesses to stack variables.

You should use ObjectSizeOffsetVisitor instead of ObjectSizeOffsetEvaluator. The interface is the similar, but it gives up when the object size is not constant, while the later may insert new instructions in the code. ObjectSizeOffsetVisitor is well tested (it's used by alias analysis, for example).

Feb 25 2015, 8:40 AM

Oct 19 2014

nlopes added a comment to D5351: Added InstCombine transform for pattern " ((X % Z) + (Y % Z)) % Z -> (X + Y) % Z "..

Thanks for clarifying that. I understand now, what that undefined behavior is for.

Another thing, in these examples, all three inputs, %x, %y, & %z are known. That is, they are constants. I think such cases wouldn't make it to the transformation. When I was testing the transformation, I tested it with these inputs, the part of the code where the transformation takes place was never reached.
The transformation is useful only when some (or all) bits of any of these inputs are unknown. Otherwise, direct calculation of the expression is done. So, there should be no change in the behavior for the above input, as, in case of this input, this transformation will never be applied.

Thanks.

Oct 19 2014, 4:46 AM
nlopes added a comment to D5351: Added InstCombine transform for pattern " ((X % Z) + (Y % Z)) % Z -> (X + Y) % Z "..

Hi nlopes,
Thanks for reviewing the patch.

In the example you have stated,

%x i2 = 3

since we are talking about signed integers, that is equivalent to %x i2 = -1

Similarly, %y i2 = -1 and %z i2 = -1.

I am not sure how [%6 = add %x, %y] calculates to [%6 i2 = 2]. Shouldn't it be [%6 i2 = -2] ?

Please clarify.
Thanks.

Oct 19 2014, 2:52 AM

Oct 11 2014

nlopes added a comment to D5351: Added InstCombine transform for pattern " ((X % Z) + (Y % Z)) % Z -> (X + Y) % Z "..

Sorry, still not correct:

Oct 11 2014, 2:58 AM

Sep 15 2014

nlopes added a comment to D5351: Added InstCombine transform for pattern " ((X % Z) + (Y % Z)) % Z -> (X + Y) % Z "..

This patch is incorrect:

Sep 15 2014, 4:50 AM