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

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

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

> 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

Yes, as I recall the intention was to explicitly encode the fact that source may not be present, and that this is distinct from an empty source file. Some changes to the corresponding Dwarf extension proposal mean this may or may not end up being representable in what becomes the final Dwarf equivalent, but I don't think that needs to dictate how we represent it here.

The overloading of pointers to `LLVMContext`-allocated types like `MDString` to mean either "definitely present, the canonical type for this is just a pointer" or "actually an `Optional<MDString>` where `nullptr`==`None`" is pretty common in the codebase, so I'm still happy if we drop the `Optional` and simply add a comment documenting that `nullptr==None`.

> 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