[PATCH] D130224: [Clang][Attribute] Introduce maybe_undef attribute for function arguments which accepts undef values

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D130224#3677261 <https://reviews.llvm.org/D130224#3677261>, @arsenm wrote:

> In D130224#3677224 <https://reviews.llvm.org/D130224#3677224>, @aaron.ballman wrote:
>
>> However, what I think I'm hearing from this thread is that there are alternative approaches that have been thought about but not tried, we're not certain how feasible those approaches are in practice, but we expect them to be materially worse than what's proposed here. So it's not "this was the path of least resistance", but "this is the correct design." Do others agree with that assessment?
>
> I think this is just the least bad on the menu of available options. I don't like it, but it at least provides documentation about this special behavior.
>
> I think the only other plausible option is to assert this is still undefined behavior and force users to update their (newly declared invalid) code. We could at least partially re-optimize to uninitialized values in the backend (although this is apparently difficult in some situations)

First off, thank you for the great discussion about this both on and off lists while I wrapped my head around the design space and challenges involved. I appreciate your willingness to explore alternatives with me!

I think I agree that this is the least bad option on the menu. The code the user wrote is undefined behavior, and the CUDA/HIP standards would like to pretend otherwise in some circumstances. Given that users will have code invalidated if we stick to the hard line language rules, I think it's better than we remove the UB than punish the user and hope they can write a more convoluted but correct form.

In terms of the patch itself, I've left some comments. But this is also missing all semantic tests (that the attribute is diagnosed when written on something other than a parameter, accept no arguments, etc). It also is missing tests for some interesting use cases:

  void func(int param);
  
  void other() {
    int derp;
    func(derp); // How should the call behave?
  }
  
  void func(__attribute__((maybe_undef)) int param) { ... }
  
  // Also:
  void foo(__attribute__((maybe_undef)) int param);
  void foo(int param) { ... } // Verify this parameter behaves as expected
  
  void bar() {
    int derp;
    foo(derp);
  }

It'd probably make sense to have a case involving templates as well to ensure that instantiation properly retains the parameter attribute, but we might already have test coverage for that (parameter attributes are pretty uncommon, but not unheard of).



================
Comment at: clang/include/clang/Basic/Attr.td:2026
 
+def MayBeUndef : InheritableAttr {
+  let Spellings = [Clang<"maybe_undef">];
----------------



================
Comment at: clang/include/clang/Basic/AttrDocs.td:260
 
+def MayBeUndefDocs : Documentation {
+  let Category = DocCatFunction;
----------------



================
Comment at: clang/include/clang/Basic/AttrDocs.td:261
+def MayBeUndefDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
----------------
I think this should be `DocCatVariable` temporarily, but we probably should add a new `DocCatParameter` category at some point and use that instead (in case you're feeling ambitious).


================
Comment at: clang/include/clang/Basic/AttrDocs.td:263
+  let Content = [{
+The ``maybe_undef`` attribute can be placed on function parameter. It indicates
+that the parameter is allowed to use undef values. It informs the compiler
----------------



================
Comment at: clang/include/clang/Basic/AttrDocs.td:265
+that the parameter is allowed to use undef values. It informs the compiler
+to insert a freeze LLVM IR instruction on the function parameter.
+
----------------



================
Comment at: clang/lib/CodeGen/CGCall.cpp:2050-2066
+static bool IsArgumentMayBeUndef(const Decl *TargetDecl, unsigned ArgNo) {
+  if (!TargetDecl)
+    return false;
+
+  bool ArgHasMayBeUndefAttr = false;
+  if (TargetDecl) {
+    if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(TargetDecl)) {
----------------
One question I have is whether you ever need to mark the variadic arguments as being maybe undef. e.g., `void func(int i, ...);` do you need to signal that arguments passed to `...` are maybe undef?



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130224/new/

https://reviews.llvm.org/D130224