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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D97251#2583843 <https://reviews.llvm.org/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).



================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:1359
     for (const auto &Base : llvm::reverse(Bases)) {
       if ((Ptr = createArgument(Arg, Attr, Base.first)))
         break;
----------------
OikawaKirie wrote:
> 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?
I believe this call is fine as-is.


================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:1364
 
+  assert(Ptr && "Cannot create argument.");
+
----------------
OikawaKirie wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > OikawaKirie wrote:
> > > > 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?
> > > 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.
> > 
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97251