[PATCH] D33368: [libcxxabi][demangler] Fix a crash in the demangler
Saleem Abdulrasool via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon May 22 21:17:20 PDT 2017
compnerd added inline comments.
================
Comment at: src/cxa_demangle.cpp:3036
break;
- if (db.names.size() < 2)
+ if (k1 <= k0)
return first;
----------------
erik.pilkington wrote:
> compnerd wrote:
> > I'm not sure how `k1` can be `<` than `k0`. Isn't this effectively always going down the `==` path? If I'm just mistaken, then I think that this needs a comment.
> I agree that it should be impossible, but the previous version handled this case with the `if (db.names.size() < 2)` check, when really `db.names.size()` can only be 1 or greater. I think it makes more sense to be paranoid here, I trust this file about as far as I can throw it (And I can't throw it very far, its not a tangible object).
Hmm, how about being paranoid, and converting that to an assertion and then doing the `==` check? This code is already incredibly complicated, so adding additional things like this only makes it harder to understand whats going on.
================
Comment at: src/cxa_demangle.cpp:3042-3051
+ for (size_t k = k0; k < k1; ++k) {
+ auto tmp = db.names[k].move_full();
+ if (!tmp.empty())
+ {
+ if (!is_first_it)
+ db.names[lambda_pos].first.append(", ");
+ is_first_it = false;
----------------
dexonsmith wrote:
> compnerd wrote:
> > I think that using a bit of algorithm here might be nice.
> >
> > std::ostringstream oss(db.names[lambda_pos]);
> > std::copy_if(db.names[k0], db.names[k1], std::ostream_iterator<std::string>(oss, ", "),
> > [](const std::string &s) -> bool { return !s.empty(); });
> Introducing a dependency on `std::ostream` would be awkward. libc++abi.dylib cannot link against libc++.dylib, and it would bloat libc++abi.dylib to use custom traits to pull in a full copy.
Ugh, thats a good point. Very well. However, there doesnt seem to be any iterator invalidation here, so we can still just iterate over a slice here, which would be nicer.
https://reviews.llvm.org/D33368
More information about the cfe-commits
mailing list