Page MenuHomePhabricator

[LangRef] Clarify undefined behavior for function attributes.

Authored by efriedma on Jul 6 2018, 1:45 PM.



Violating the invariants specified by attributes is undefined behavior. Maybe we could use poison instead for some of the parameter attributes, but I don't think it's worthwhile.

Diff Detail


Event Timeline

efriedma created this revision.Jul 6 2018, 1:45 PM
nlopes added inline comments.Jul 8 2018, 8:05 AM
1540 ↗(On Diff #154450)

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?

sanjoy added inline comments.Jul 8 2018, 10:47 AM
1540 ↗(On Diff #154450)

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.

hfinkel added inline comments.Jul 8 2018, 2:47 PM
1540 ↗(On Diff #154450)

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.

efriedma added inline comments.Jul 9 2018, 12:55 PM
1540 ↗(On Diff #154450)

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.

hfinkel added inline comments.Jul 10 2018, 9:21 AM
1540 ↗(On Diff #154450)

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.

This revision is now accepted and ready to land.Jul 19 2018, 10:01 AM
This revision was automatically updated to reflect the committed changes.