[PATCH] D111417: [Demangle] Add support for D symbols back referencing

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:136-147
+  /// Extract the back reference position from a given string
+  ///
+  /// \param Mangled string to extract the back reference position
+  /// \param Ret assigned result value
+  ///
+  /// \return the remaining string on success or nullptr on failure
+  ///
----------------
ljmf00 wrote:
> dblaikie wrote:
> > Perhaps it'd make sense if this took Mangled as a const char** in/out, and returned Optional<Ret>? (both callers overwrite their Mangled parameter with the return result anyway - so seems simpler for it to be in/out?) if Optional isn't available, maybe long min, -1, something could be the invalid result value?
> > 
> > (similarly for decodeBackref)
> As is, I understand your point, but what if I want to preserve Mangled and use `decodeBackref`/`decodeBackrefPos` as an expression for an if, strcmp or else? e.g. On the patch I introduce integer literal values I use `decodeBackref` like the following:
> ```
> if (decodeBackref(Mangled, &Backref) == nullptr)
> {
>     //...
> }
> ```
> If I pass a reference of Mangled and use old Mangled inside, I would need to preserve a copy of its old position to make sure it won't be a "dirty" value. e.g. back reference "AAAA0", will produce a dirty Mangled and can produce bad behaviour afterwards.
Fair enough, if there are future use cases that won't immediately overwrite `Mangled`, sounds good.

`Ret` should probably be passed by reference here, and in `decodeBackref` if they can't be null?


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:386-387
+      Val += Mangled[0] - 'a';
+      if ((long)Val <= 0)
+        break;
+      *Ret = Val;
----------------
ljmf00 wrote:
> dblaikie wrote:
> > is this an overflow check? Can it be reached if the other overflow check at 379 succeeds? (is it tested?)
> This is a safe check when casting to long. The above overflow check is for unsigned long, but Ret is a long value. This is being done that way because signed overflow checks are considered undefined behaviour, since there is various ways to represent a signed value in different CPU targets. I checked locally, `_D8demangleQDXXXXXXXXXXXXx` triggers it with the fuzzer when that code is commented, so I'm going to add this to the test suite.
The fuzzer trips ubsan with this `<= 0` check commented out, for this test case? OK, great - thanks for explaining & adding the test case!

Hmm, I'm still not quite sure how this overflow could occur, if the check on line 214 passed. Could you walk me through a specific value of `Val` at the start of the loop that passes the initial overflow check, but then fails this one? And/or could you explain what the `- 25` is for in the earlier overflow check. I'm reading that as "make sure it's not too close to the end, so there's room to add the next value" but maybe it's for some other purpose?


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:402-403
+
+  if (Mangled == nullptr || *Mangled != 'Q')
+    return nullptr;
+
----------------
ljmf00 wrote:
> dblaikie wrote:
> > Looks like this condition's probably untested? Should it be removed or turned into an assertion?
> Well this shouldn't happen and since those methods are not exposed somewhere else, the compiler can optimize this out easily. I think it is reasonable to keep this check as a safety measure, although if we do some action, I maybe prefer to assert it. In my vision, triggering an assert on, e.g. a failed demangling for a linker error is worse than just outputting the mangled symbol anyway (`nullptr` will make `llvm::demangle` fallback to the initial Mangled reference). It is also worth mentioning that removing this when assert are disabled in release mode, can produce wrong and hard to debug behaviour.
Sorry, not sure I'm following. Generally if the code can't be reached, it should be an assert, not a runtime check - adding defensive coding to support behavior if there are other bugs in the code isn't something we should be doing. This ensures the code doesn't have "haunted graveyard" kind of effects where code becomes hard to maintain because it's unclear why it's there in the first place, isn't tested, etc. Assertions document the intended invariants in a piece of code that make it easier for developers to make changes in the future.

>  It is also worth mentioning that removing this when assert are disabled in release mode, can produce wrong and hard to debug behaviour.

It can produce wrong/hard to debug behavior if there's some other bug in the code, yes? Generally the first thing LLVM developers do if they encounter problematic behavior, is run it with an assertions-enabled build which should help diagnose the issue. There's some further discussion here: https://llvm.org/docs/CodingStandards.html#assert-liberally


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

https://reviews.llvm.org/D111417