[PATCH] D138658: [DebugInfo] Protect DIFile::Source against empty string

[Jonas Hahnfeld via Phabricator via llvm-commits]

Hahnfeld added a comment.

In D138658#3951866 <https://reviews.llvm.org/D138658#3951866>, @dblaikie wrote:

> Where did this issue come up in practice? Change seems reasonable, but wouldn't mind having more context.

Please see https://reviews.llvm.org/D137152 for context. I think what's happening is that we (the Cling interpreter in ROOT) create an empty source file while injecting some code during startup. It may be possible to fix that, but LLVM shouldn't crash from it.



================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:658-661
+    if (!Source)
+      return None;
+    assert(*Source && "nullptr in Optional?");
+    return Optional<StringRef>((*Source)->getString());
----------------
dblaikie wrote:
> How is this different from the previous code?  Optional::operator* would have an assert in it, right? So the previous code would've been asserting in the same places/ways, I think?
Yes, there is an `assert` in `OptionalStorage::value`, but it only checks that there is a value. `nullptr` is a valid value inside a `Optional<MDString *>`, but the expectation of this class is that `Source` is either `None` or contains a non-null pointer.

(Side remark: It //may// be possible to change `Source` to just a `MDString *` and make `nullptr` mean that there is no source. That's equally fine with me, but requires changing the signature of `getRawSource`. Let me know if you prefer that.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138658