[PATCH] D34667: [Demangler] [DO NOT SUBMIT] Initial patch for Microsoft demangler.

[Zachary Turner via Phabricator via llvm-commits]

zturner added a comment.

More comments later when I have some time to look in more detail.



================
Comment at: llvm/lib/Demangle/MicrosoftDemangle.cpp:95
+  Fastcall,
+  Regcall,
+};
----------------
What about `vectorcall`?


================
Comment at: llvm/lib/Demangle/MicrosoftDemangle.cpp:303-313
+// Sometimes numbers are encoded in mangled symbols. For example,
+// "int (*x)[20]" is a valid C type (x is a pointer to an array of
+// length 20), so we need some way to embed numbers as part of symbols.
+// This function parses it.
+//
+// <number>               ::= [?] <non-negative integer>
+//
----------------
Can you add a similar comment above the `parse()` function that describes the high level grammar, similar to how you've done here with this one subsection?


================
Comment at: llvm/lib/Demangle/MicrosoftDemangle.cpp:331
+    }
+    if ('A' <= c && c <= 'P') {
+      ret = (ret << 4) + (c - 'A');
----------------
What is the significance of `'A'` and `'P'`?


================
Comment at: llvm/lib/Demangle/MicrosoftDemangle.cpp:411-412
+    return Fastcall;
+  if (consume("E"))
+    return Regcall;
+  error = "unknown calling convention: " + input.str();
----------------
`vectorcall` is `Q`


================
Comment at: llvm/lib/Demangle/MicrosoftDemangle.cpp:418
+// <return-type> ::= <type>
+//               ::= @ # structors (they have no declared return type)
+void Demangler::read_func_return_type(Type &ty) {
----------------
`s/structors/destructors/`?  Otherwise I'm not sure what this means.


================
Comment at: llvm/lib/Demangle/MicrosoftDemangle.cpp:428
+
+int8_t Demangler::read_storage_class() {
+  if (consume("A"))
----------------
Not all of these are valid for all types.  For example, `unaligned` is not valid for functions.  Do we care about this?


================
Comment at: llvm/lib/Demangle/MicrosoftDemangle.cpp:447-448
+    return Huge;
+  if (consume("F"))
+    return Unaligned;
+  if (consume("I"))
----------------
This is not quite right.  After the `F` comes an additional storage class.  For example, if it's simply unaligned, it will be `FA`.  `const __unaligned` will be `FB`.  `const volatile __unaligned` would be `FD`.  etc.


================
Comment at: llvm/lib/Demangle/MicrosoftDemangle.cpp:449-450
+    return Unaligned;
+  if (consume("I"))
+    return Restrict;
+  return 0;
----------------
Same is true here.  You need to also consume the following character and or in the result until you get a terminating sequence


================
Comment at: llvm/lib/Demangle/MicrosoftDemangle.cpp:455
+// Reads a variable type.
+void Demangler::read_var_type(Type &ty) {
+  if (consume("T")) {
----------------
Can we have another grammar summary here?


================
Comment at: llvm/lib/Demangle/MicrosoftDemangle.cpp:491-504
+  if (consume("PEA")) {
+    ty.prim = Ptr;
+    ty.ptr = alloc();
+    read_var_type(*ty.ptr);
+    return;
+  }
+
----------------
majnemer wrote:
> Isn't 'E' only for 64-bit?
This is not quite right.  This `A` and `B` here corresponds to the storage class returned from `read_storage_class()`.  So you need to be prepared to handle every `PE` + <any possible return value of `read_storage_class()`>, including the complex return values like `FA` and `FIB`, etc.


================
Comment at: llvm/lib/Demangle/MicrosoftDemangle.cpp:506
+
+  if (consume("QEB")) {
+    ty.prim = Ptr;
----------------
Same is true here.


https://reviews.llvm.org/D34667