[PATCH] D56721: Move llvm-objdump demangle function into demangler library

[James Henderson via Phabricator via llvm-commits]

jhenderson added a reviewer: erik.pilkington.
jhenderson added inline comments.


================
Comment at: lib/Demangle/Demangle.cpp:20
+std::string demangle(char const *MangledName) {
+  assert (MangledName != nullptr);
+
----------------
grimar wrote:
> This line does not seem to be clang-formatted, but I also think it is not useful.
> In case MangledName is null, you'll have a crash anyways, the crash is an obvious assert by itself.
> From what I saw in LLVM's code, assert is more commonly used to check the more complex logic flows, so I would remove it. 
This protects us against undefined behaviour in strncmp below. It may not be a crash if nullptr is passed in.


================
Comment at: lib/Demangle/Demangle.cpp:23
+  char *Demangled = nullptr;
+  if (strncmp(MangledName, "_Z", 2) == 0)
+    Demangled = itaniumDemangle(MangledName, Demangled, nullptr, nullptr);
----------------
grimar wrote:
> erik.pilkington wrote:
> > You should probably also send strings that start with "___Z" to the itanium demangler.
> This patch just moves the existent code, I would not add any additional functionality in it.
I agree, though I don't mind it being in a follow-up change, if desired. The main advantage of this change as is, is that it is a pure refactor (there might be some minor differences in performance, but the end behaviour should be identical to what llvm-objdump previously did).


================
Comment at: lib/Demangle/Demangle.cpp:26
+  else
+    Demangled = microsoftDemangle(MangledName, Demangled, nullptr, nullptr);
+
----------------
grimar wrote:
> erik.pilkington wrote:
> > nit: may as well just pass in `nullptr` instead of `Demangled`.
> The same.
In this case, passing in `nullptr` is identical to passing in `Demangled`. It's safe to change.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:531
+        assert(SymName->end() < StrTab.end() && *SymName->end() == '\0');
+        Target = demangle(SymName->data());
+      }
----------------
erik.pilkington wrote:
> StringRef::data doesn't guarantee a null-terminator. This may not be a problem in practice here, but to be safe you should call demangle(Name.str().c_str()). Alternatively, making demangle take a std::string probably wouldn't be the end of the world, since it isn't the "high performance" demangling API anyways since it copies the result buffer (which is likely much larger than then the input).
In this particular case, that was the purpose of the earlier assert. However, in the other two places, it's not straightforward to construct an assert that checks: although we know that the string tables in practice are null-terminated currently (and checked as such), this is not easily proven in this area.

I think being safe is important, so I'll change to the `.str().c_str()` suggestion, and if there's a clear performance issue, it can be revisited later.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56721