[PATCH] D49041: [LangRef] Clarify undefined behavior for function attributes.

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: docs/LangRef.rst:1540
+    If a readnone function reads or writes memory visible to the program, or
+    has other side-effects, the behavior is undefined. If a function reads from
+    or writes to a readnone pointer argument, the behavior is undefined.
----------------
efriedma wrote:
> hfinkel wrote:
> > sanjoy wrote:
> > > nlopes wrote:
> > > > uhm, in the way it's written, functions marked with readnone, readonly, writeonly, argmemonly cannot e.g. divide by 0. So either all divisions are guarded to ensure that they never trigger UB, or functions with potentially unsafe divisions cannot be marked as *only.
> > > > Is this intended?
> > > > 
> > > > For readnone/readonly I can see the motivation: this allows hoisting these functions safely (modulo the non-returning functions problem).
> > > > For write-only and argmemonly, what's the motivation to block any other UB?
> > > > 
> > > > Should the has-no-UB bit be split into a separate flag? Are there use cases for the clients of readnone/readonly? I guess at least ModRef could benefit of a readonly without the no-UB bit?
> > > We should also state that these properties need to hold for all inputs to the function.
> > > 
> > > I too think the no-UB property should have a different bit, otherwise we will have a harder time inferring `readnone` -- we'd have to do some control flow analysis to ensure that we don't branch on inputs that could possibly be poison and that we don't have (UB via) infloops.
> > I also agree. We need a separate attribute for the no-UB part. I'd like to have it, but it will be a breaking change to add to the semantics for readnone.
> The intent here is that a function can have undefined behavior even if it's "readnone"; undefined behavior isn't a side-effect.  (The "other side-effects" bit is supposed to cover external state changes, like a syscall, which might not modify memory visible to the process.)  Do you have a suggested rewrite to make that more clear?
> 
> We already have a "speculatable" attribute to mark a function as never-UB; some intrinsics have this marking.
> We already have a "speculatable" attribute to mark a function as never-UB; some intrinsics have this marking.

Sadly, I'd completely forgotten that we had finally added this attribute. We aren't inferring it, however. A patch to do that: D49144.



Repository:
  rL LLVM

https://reviews.llvm.org/D49041