This is an archive of the discontinued LLVM Phabricator instance.

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.
ClosedPublic

Authored by rsmith on Aug 20 2018, 3:19 PM.

Diff Detail

Event Timeline

rsmith created this revision.Aug 20 2018, 3:19 PM

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.

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.

(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.)

erik.pilkington accepted this revision.Aug 23 2018, 1:15 PM

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.

include/llvm/Demangle/ItaniumDemangle.h
2788

Missed a spot here too!

2852

Might be nice to inline the nullptr check on Comp into this function too.

2888

Missed a spot!

4773

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.

This revision is now accepted and ready to land.Aug 23 2018, 1:15 PM
rsmith marked 4 inline comments as done.Aug 24 2018, 3:48 PM
rsmith added inline comments.
include/llvm/Demangle/ItaniumDemangle.h
2905–2915

Incidentally: this doesn't look quite right to me. A <substitution> can only appear at the start of a <prefix>, right? (We should presumably parse this before the loop, like we do for St.)

This revision was automatically updated to reflect the committed changes.