[PATCH] D51003: Allow demangler's node allocator to fail, and bail out of the entire demangling process when it does. Use this to support a "lookup" query for the mangling canonicalizer that does not create new nodes.

[Erik Pilkington via Phabricator via llvm-commits]

erik.pilkington accepted this revision.
erik.pilkington added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/D51003#1210421, @rsmith wrote:

> In https://reviews.llvm.org/D51003#1208827, @erik.pilkington wrote:
>
> > It seems like we would need to add a lot more nullptr checks in order to really support this then this patch does. One potential workaround is to add a subclass of Node in ItaniumManglingCanonicalizer.cpp that serves as the canonical 'sentinel' node. We could just hand that out whenever we would otherwise allocate anything, and ignore the results of parse() if we ever did. I think that would probably maybe work.
>
>
> This patch fixes up every place where we create a node and do not immediately return it; all parsing functions returning a `Node*` are already assumed to potentially fail, so their callers already perform a null check. So I think this is the full extent of what we need to do to support this.


Oh, right, my mistake. LGTM after some inline comments.

> (If we wanted to actually make the demangler be able to run with a fixed size buffer, there are two other things we need to address: NodeArray allocation and the explicit malloc/free calls made by PODSmallVector. Those both seem feasible, if we ever want to actually go there.)

Ya, I think that would be a good thing to do. It'd also allow us to more gracefully handle failed allocation (right new we just std::terminate()), which would be nice.



================
Comment at: include/llvm/Demangle/ItaniumDemangle.h:2788
     case SpecialSubKind::iostream:
       SoFar = make<ExpandedSpecialSubstitution>(SSK);
     default:
----------------
Missed a spot here too!


================
Comment at: include/llvm/Demangle/ItaniumDemangle.h:2852
   Node *SoFar = nullptr;
   auto PushComponent = [&](Node *Comp) {
     if (SoFar) SoFar = make<NestedName>(SoFar, Comp);
----------------
Might be nice to inline the nullptr check on `Comp` into this function too.


================
Comment at: include/llvm/Demangle/ItaniumDemangle.h:2888
         return nullptr;
       SoFar = make<NameWithTemplateArgs>(SoFar, TA);
       if (State) State->EndsWithTemplateArgs = true;
----------------
Missed a spot!


================
Comment at: include/llvm/Demangle/ItaniumDemangle.h:4773
     }
     Attrs = make<EnableIfAttr>(popTrailingNodeArray(BeforeArgs));
   }
----------------
You should check here too. It won't result in a crash, but it'll cause lookup to return a non-null key when it shouldn't.


Repository:
  rL LLVM

https://reviews.llvm.org/D51003