[llvm] Add assertions for the smart pointers with the possibility to be null in ClangAttrEmitter

[OikawaKirie]

Summary

Split from D91844.

The return value of createArgument in file clang/utils/TableGen/ClangAttrEmitter.cpp. Although there are a lot of checks in the function, nullptr is still possible to be returned.
In this patch, I added an assertion to confirm that.

[OikawaKirie]

As a recursive function it is, the original patch in D91844 was adding assertions where the function is called other places in the file.

To confirm it will not lead to a crash here, I re-build my code base with clang and clang-tools-extra enabled. Everything seems to be good.

What do you think about the assertion? Should it be located just here or where the function is called?

Refer to D91844 for more details about the previous version.

clang/utils/TableGen/ClangAttrEmitter.cpp

1364

Just as what has been suggested by @dexonsmith in D91844, I add the assertion here. However, as a recursive function it is (line 1359), I still concern whether the assertion here will lead to a crash.

What do you think?

[aaron.ballman]

LGTM with a few minor changes to be made. Thank you for the cleanup!

clang/utils/TableGen/ClangAttrEmitter.cpp

1364

The intent with this function is that it never returns nullptr unless the programmer messed up their call of the function (passed in the wrong kind of Record kind of problem). All of the callers assume this returns non-null, so I think it's reasonable to assert that the value is non-null by this point.

1364

1366–1370

[OikawaKirie]

@aaron.ballman

My only concern is the recursive call to this function on line 1359. If you think it is also OK for the recursive call on line 1359, I will update the patch as you mentioned above.

BTW, how can I test and debug this tblgen backend?

clang/utils/TableGen/ClangAttrEmitter.cpp

1359

The recursive call is here.

The for loop here seems to allow a failed attempt on its bases. Although the top-level callers expect the return value of this function is always non-null, what about the call here?

1364

However, there is a recursive call to this function on line 1359, and it seems that nullptr is allowed to be returned for the call. I am just worrying about whether the assertion here will lead to a crash in the recursive calls.

IMO, a better way is to wrap this function and assert the return value of this function in the wrapper function.

[aaron.ballman]

In D97251#2583843, @OikawaKirie wrote:

@aaron.ballman

My only concern is the recursive call to this function on line 1359. If you think it is also OK for the recursive call on line 1359, I will update the patch as you mentioned above.

BTW, how can I test and debug this tblgen backend?

There really is no way to test these changes, but you can debug the generator by running clang-tblgen -gen-whatever-you-want -I path\to\clang\include path\to\clang\include\clang\Basic\Attr.td (the available generators are listed in clang\utils\TableGen\TableGen.cpp).

clang/utils/TableGen/ClangAttrEmitter.cpp

1359

I believe this call is fine as-is.

1364

I don't think we need to go to those lengths -- if the recursive call fails, there was a Clang programmer mistake (they wrote the wrong thing in Attr.td or they forgot to write the right things in ClangAttrEmitter.cpp).

Put another way: we could use PrintFatalError(Arg.getLoc(), "Failed to create the argument") instead of assert if we wanted.

clang/utils/TableGen/ClangAttrEmitter.cpp

else if (ArgName == "VersionArgument")
     Ptr = std::make_unique<VersionArgument>(Arg, Attr);
   else if (ArgName == "OMPTraitInfoArgument")
     Ptr = std::make_unique<SimpleArgument>(Arg, Attr, "OMPTraitInfo *");
 
   if (!Ptr) {
     // Search in reverse order so that the most-derived type is handled first.
     ArrayRef<std::pair<Record*, SMRange>> Bases = Search->getSuperClasses();
     for (const auto &Base : llvm::reverse(Bases)) {
       if ((Ptr = createArgument(Arg, Attr, Base.first)))
         break;
     }
   }
 
+  assert(Ptr && "Cannot create argument.");
+
   if (Ptr && Arg.getValueAsBit("Optional"))
     Ptr->setOptional(true);
 
   if (Ptr && Arg.getValueAsBit("Fake"))
     Ptr->setFake(true);
 
   return Ptr;
 }
 
 static void writeAvailabilityValue(raw_ostream &OS) {
   OS << "\" << getPlatform()->getName();\n"
      << "  if (getStrict()) OS << \", strict\";\n"
      << "  if (!getIntroduced().empty()) OS << \", introduced=\" << getIntroduced();\n"