[LangRef] Add `noundef` attribute to documentation

[guiand]

Summary

[LangRef] Introduce noundef attribute for fully-defined function params

LLVM currently does not require function parameters or return values
to be fully initialized, and does not care if they are poison. This can
be useful if the frontend ABI makes no such demands, but may prevent
helpful backend transformations in case they do. Specifically, the C
and C++ languages require all scalar function operands to be fully
determined.

Introducing this attribute is of particular use to MemorySanitizer
today, although other transformations may benefit from it as well.
We can modify MemorySanitizer instrumentation to provide modest (17%)
space savings where frozen is present.

This commit only adds the attribute to the Language Reference, and
the actual implementation of the attribute will follow in a separate
commit.

[nikic]

llvm/docs/LangRef.rst

1255

Should be just frozen, without the quotes. Quotes indicate a string attribute.

1259

This description is a bit too frontend-centric. I'd suggest to at least add

If the parameter is poison or contains undefined bits, the behavior is undefined.

to describe the actual IR semantic this has.

Also, can this be applied to return values? If so, it should say so.

[jdoerfert]

Thanks for splitting this off!

I'm still in favor of nopoison as a name but so far no one else supports that name it seems. At least I would prefer that name if this means "not poison or instant UB".
Now, the documentation talks about undefined bits, which is something different. Maybe frozen makes sense after all.

---

As mentioned by @nikic, we should not talk about ABI, frontends, backends, etc. in the lang ref if not strictly necessary. I think describing the attribute as the absence of undefined or poison bits in the value representation is what we want :)

llvm/docs/LangRef.rst

3666

Can we make this two bullet points.
Return values are not really operands so maybe: "The operand of a return instruction if the function or invoking call site has a `frozen` attribute in the return value position".
I would also not talk about Arguments here as we describe instructions. So maybe say "The operand of a :ref:call <i_call> or :ref:invoke <i_invoke> instruction for which the call site has a `frozen` attribute at the respective position."

[eugenis]

How about "noundef" for a name? I'm not a fan of names that start with "no", but "frozen" is not so obvious at a glance.

[aqjune]

noundef seems better to me, as it means the value can't be poison as well (because a poison value can change to undef).
I'm fine with the suggestion as far as it is added to LangRef that the value is frozen if noundef is attached and it isn't an aggregate type with padding bits (if definition is changed as I suggested).

llvm/docs/LangRef.rst

1259

For aggregate types, I think it is okay to relax this constraint to allowing aggregates with undef padding bits. It will be great if an aggregate value that is constructed with a chain of v = insertvalue (insertvalue undef, const1, idx), const2, idx2 can have this attribute attached regardless of whether the aggregate type has paddings or not. Inspecting padding bits which are not accessible without using memory operation may be overly restrictive.
For my previous concern (giving a guarantee that it has no undef bit in memory representation), this should be needed in certain future cases, but not now maybe.

[eugenis]

llvm/docs/LangRef.rst

1259

Yes, that's a good point. From MSan point of view, if {i8, i32} is passed as a call argument, we don't care about the padding in the middle - the type definition has all we need to understand which bits must be checked for definedness, and we'd like the attribute to reflect that - i.e. the property of the IR value itself and not its storage representation.

(if it's coerced to i64, then we care).

[nlopes]

llvm/docs/LangRef.rst

1260

What happens when this condition is violated? e.g., when one passes a poison value as argument? Do we get UB? (I think so, but it's worth to make it clear)

[jdoerfert]

llvm/docs/LangRef.rst

1259

Does this mean we still need more than nopoison?

1260

Yes, this should be one of the only "value attribute" that will cause UB. The others should poison the value on violation and combined with this one you get UB.

[eugenis]

llvm/docs/LangRef.rst

1259

Sorry, I don't understand - are saying the attribute can be specified in terms of poison values and not undef? I don't think it would work, we need it to cover something that's loaded from an uninitialized memory location and passed to a function call as is, and those are undef, not poison.

[aqjune]

llvm/docs/LangRef.rst

1259

Yes, we need an attribute for undef bits; a constraint for absence of poison may not be enough.
In the long term, I'd prefer merging the notion of undef value poison value into one (by leaving poison only), then nopoison should be enough. But seems like it is a pretty tough way to go.

[jdoerfert]

noundef seems better to me, as it means the value can't be poison as well (because a poison value can change to undef).

+1

Other comments in this patch still need to be addressed as well.

[guiand]

Changed attribute name from frozen to noundef, addressed comments

[arsenm]

llvm/docs/BitCodeFormat.rst

1061

Can you commit this documentation change for the existing attributes separately

[nlopes]

llvm/docs/LangRef.rst

1257

can we write undef instead of uninitialized pls? LLVM doesn't really have uninitialized values.
Otherwise LGTM.

[nikic]

llvm/docs/LangRef.rst

1258

I don't understand what "inferable" is intended to mean in this context.

[guiand]

llvm/docs/LangRef.rst

1258

Quoting from other comments here:

For aggregate types, I think it is okay to relax this constraint to allowing aggregates with undef padding bits. It will be great if an aggregate value that is constructed with a chain of v = insertvalue (insertvalue undef, const1, idx), const2, idx2 can have this attribute attached regardless of whether the aggregate type has paddings or not. Inspecting padding bits which are not accessible without using memory operation may be overly restrictive.

From MSan point of view, if {i8, i32} is passed as a call argument, we don't care about the padding in the middle - the type definition has all we need to understand which bits must be checked for definedness, and we'd like the attribute to reflect that - i.e. the property of the IR value itself and not its storage representation.

This is what I tried to capture with this description. Is there some better way I might express this succinctly?

[efriedma]

llvm/docs/LangRef.rst

1258

Not sure we have any existing terminology to describe this concept.

C++ calls these bits "pading bits" (vs. the "value representation"). Maybe we need a section in LangRef to describe this. Probably doesn't belong specifically in the discussion of the meaning of noundef, though, since it applies more generally. Maybe add some description in the definition of struct/array types, and link to it from here.

[nikic]

llvm/docs/LangRef.rst

1258

I think if you replace "inferable" with "inaccessible" or "unobservable", that would be more obvious. At least, I don't think there is any way to access/observe these padding bits or even their existence from LLVM IR (without a pre-existing pointer indirection).

[jdoerfert]

I think, we could even remove the padding sentence and say "undefined or poison" bits. The latter seems to match our existing language. The former should be OK because a value of type { i8, i32 } has 8 + 32 bits. The fact that the store and alloca size is different does not mean the value has undefined bits. If people agree with this interpretation we could also add a sentence explaining this difference.

[eugenis]

In D82316#2117857, @jdoerfert wrote:

I think, we could even remove the padding sentence and say "undefined or poison" bits. The latter seems to match our existing language. The former should be OK because a value of type { i8, i32 } has 8 + 32 bits. The fact that the store and alloca size is different does not mean the value has undefined bits. If people agree with this interpretation we could also add a sentence explaining this difference.

That matches my understanding as well.

We could also add a note that value bits here explicitly do NOT mean storage representation to avoid any misunderstanding.

[guiand]

Addressed comments

[jdoerfert]

LGTM. Thanks for working on this. This was needed for a long while and I think the new wording is proper.

[jdoerfert]

reverse ping. Let's land this :)

[guiand]

Should I land this without waiting for the other two patches?

[jdoerfert]

In D82316#2137939, @guiand wrote:

Should I land this without waiting for the other two patches?

Either way works. Don't want us to forget to land it ;)

[nikic]

In D82316#2137939, @guiand wrote:

Should I land this without waiting for the other two patches?

I'd suggest to land this together with the LLVM part of D81678 (which LGTM) and land the clang part that actually starts using this and requires the massive test changes separately.

llvm/docs/BitCodeFormat.rst

 * code 51: ``allocsize(<EltSizeParam>[, <NumEltsParam>])``
 * code 52: ``writeonly``
 * code 53: ``speculatable``
 * code 54: ``strictfp``
 * code 55: ``sanitize_hwaddress``
 * code 56: ``nocf_check``
 * code 57: ``optforfuzzing``
 * code 58: ``shadowcallstack``
 * code 59: ``speculative_load_hardening``
 * code 60: ``immarg``
 * code 61: ``willreturn``
 * code 62: ``nofree``
 * code 63: ``nosync``
 * code 64: ``sanitize_memtag``
 * code 65: ``preallocated``
 * code 66: ``no_merge``
 * code 67: ``null_pointer_is_valid``
+* code 68: ``noundef``
 
 .. note::
   The ``allocsize`` attribute has a special encoding for its arguments. Its two
   arguments, which are 32-bit integers, are packed into one 64-bit integer value
   (i.e. ``(EltSizeParam << 32) | NumEltsParam``), with ``NumEltsParam`` taking on
   the sentinel value -1 if it is not specified.
 
 .. _TYPE_BLOCK:

llvm/docs/LangRef.rst

 
 ``immarg``
     This indicates the parameter is required to be an immediate
     value. This must be a trivial immediate integer or floating-point
     constant. Undef or constant expressions are not valid. This is
     only valid on intrinsic declarations and cannot be applied to a
     call site or arbitrary function.
 
+``noundef``
+    This attribute applies to parameters and return values. If the value
+    representation contains any undefined or poison bits, the behavior is
+    undefined. Note that this does not refer to padding introduced by the
+    type's storage representation.
+
 .. _gc:
 
 Garbage Collector Strategy Names
 --------------------------------
 
 Each function may specify a garbage collector strategy name, which is simply a
 string:
 

 -  The pointer operand of a :ref:`load <i_load>`, :ref:`store <i_store>` or
    any other pointer dereferencing instruction (independent of address
    space).
 -  The divisor operand of a ``udiv``, ``sdiv``, ``urem`` or ``srem``
    instruction.
 -  The condition operand of a :ref:`br <i_br>` instruction.
 -  The callee operand of a :ref:`call <i_call>` or :ref:`invoke <i_invoke>`
    instruction.
+-  The parameter operand of a :ref:`call <i_call>` or :ref:`invoke <i_invoke>`
+   instruction, when the function or invoking call site has a ``noundef``
+   attribute in the corresponding position.
+-  The operand of a :ref:`ret <i_ret>` instruction if the function or invoking
+   call site has a `noundef` attribute in the return value position.
 
 Here are some examples:
 
 .. code-block:: llvm
 
     entry:
       %poison = sub nuw i32 0, 1           ; Results in a poison value.
       %still_poison = and i32 %poison, 0   ; 0, but also poison.