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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a subscriber: scott.linder.
dblaikie added a comment.

In D138658#3960331 <https://reviews.llvm.org/D138658#3960331>, @probinson wrote:

>> A zero-length file isn't valid in C++ (you have to have a newline at the end)
>
> LLVM is used for many languages, and they might not all have that requirement.  Also, I just tried running `clang -c -g` on a zero-length file; it didn't issue any diagnostics at all, and produced an object file (although it did not have any debug-info sections).

Sure enough. Does look like the Source parameter was intended to be handled differently than other strings (like Name) - other strings are passed in as StringRef, and empty is the null/not-present state, but Source was specifically wrapped in Optional in/out presumably to express a distinction between empty and not present. @scott.linder - do you recall if/how intentional this was? If it is as it looks, then

Old/currently committed code:
When constructing the DIFile and the Source is:
None -> None
Empty -> non-None null pointer
Non-empty -> non-None non-null pointer
When accessing the source:
None -> None
Empty -> crash
Non-empty -> non-None non-null pointer

The new behavior of the current patch:
Empty -> None
Access:
None -> None

So this patch collapses empty with not present - and if that's what we wanted to do, the way to do that would be to remove the Optional entirely - and store Source the same way we store other strings in MDOperands, and pass it around as a StringRef with empty as none and skip the Optional indirection.

If we're keeping the distinction between empty and not-present, then we need to choose which representation we use - we could use non-None null pointer to represent empty, or we could use non-None-non-null to a zero-length MDString. I guess the former is easier and only requires a fix in the accessor, to map non-None-null to Optional<StringRef>("") basically? So maybe do that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138658