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

[Jonas Hahnfeld via Phabricator via llvm-commits]

Hahnfeld added a comment.

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

> In D138658#3951887 <https://reviews.llvm.org/D138658#3951887>, @Hahnfeld wrote:
>
>> 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.
>
> hmm, more context might be nice - like where's the Cling code that's doing something different than the Clang code when it's creating DIFiles?

I think it's actually Clang code that just passes on the result of `getSource`: https://github.com/llvm/llvm-project/blob/fb2f3b30b2ab2494aedf54940f80258e073f0486/clang/lib/CodeGen/CGDebugInfo.cpp#L426 which is not protected against empty strings: https://github.com/llvm/llvm-project/blob/fb2f3b30b2ab2494aedf54940f80258e073f0486/clang/lib/CodeGen/CGDebugInfo.cpp#L376-L388

> I think it might be more suitable to say that DIFile shouldn't be given a non-None Optional that refers to an empty string? & it's up to the caller (Cling) to fix that - we could add an assertion to that effect in the getImpl?
>
> Or perhaps there's an intent to support present-but-zero-length source, in which case this change isn't suitable either, I guess? (since this change collapses the empty and None cases together)
>
> If None and empty are meant to be identical, maybe removing the Optional layer entirely and using the empty string to communicate not-present would be suitable? I'm not sure.

Well yes, these are the other three possibilities we have. I'm fine with any and went with the least intrusive, but I can implement any of them if they are more suitable from a DebugInfo point of view (I'm not an expert there).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138658