[PATCH] Add a DIExternalTypeRef debug metadata node to the IR.
Duncan P. N. Exon Smith
dexonsmith at apple.com
Fri May 8 12:04:25 PDT 2015
> On 2015 May 8, at 14:18, Adrian Prantl <aprantl at apple.com> wrote:
>
> Hi dexonsmith, dblaikie, echristo,
>
> This adds a DIExternalTypeRef debug metadata node to the IR that is meant to be emitted by Clang when referring to a type that is defined in a .pcm file. DIExternalTypeRef is a child of DICompositeTypeBase because it shares common traits such as the unique identifier.
>
> The idea how this is going to be used is that the frontend will discover that a type originates from an AST file and instead of and constructing a full DIType (and thus deserializing the type from the AST) it creates a DIExternalTypeRef with the tag type, mangled name and the .pcm file. The backend then can emit this as a split-DWARF-style forward declaration using DW_FORM_ref_sig8 (GDB) or as a string reference (LLDB) + and accelerator table entry.
>
> REPOSITORY
> rL LLVM
>
> http://reviews.llvm.org/D9612
>
> Files:
> include/llvm/Bitcode/LLVMBitCodes.h
> include/llvm/IR/DIBuilder.h
> include/llvm/IR/DebugInfoMetadata.h
> include/llvm/IR/Metadata.def
> include/llvm/IR/Metadata.h
> lib/AsmParser/LLParser.cpp
> lib/Bitcode/Reader/BitcodeReader.cpp
> lib/Bitcode/Writer/BitcodeWriter.cpp
> lib/IR/AsmWriter.cpp
> lib/IR/DIBuilder.cpp
> lib/IR/DebugInfoMetadata.cpp
> lib/IR/LLVMContextImpl.h
> lib/IR/Verifier.cpp
> test/Assembler/DIExternalTypeRef.ll
>
> EMAIL PREFERENCES
> http://reviews.llvm.org/settings/panel/emailpreferences/
> <D9612.25348.patch>
This looks mostly right to me; just a bunch of low-level things.
> Index: lib/AsmParser/LLParser.cpp
> ===================================================================
> --- lib/AsmParser/LLParser.cpp
> +++ lib/AsmParser/LLParser.cpp
> @@ -3529,6 +3529,21 @@
> return false;
> }
>
> +/// ParseDIExternalTypeRef:
> +/// ::= !DIExternalTypeRef(tag: DW_TAG_class_type, module: !1, identifier: "UUID")
Comment seems wrong here. I think you mean 'file'.
> +bool LLParser::ParseDIExternalTypeRef(MDNode *&Result, bool IsDistinct) {
> +#define VISIT_MD_FIELDS(OPTIONAL, REQUIRED) \
> + REQUIRED(tag, DwarfTagField, ); \
> + REQUIRED(file, MDField, ); \
Should this use `/* AllowNull */ false`? Otherwise, should this be
`OPTIONAL`? ("No" to both questions is possible, but a little strange.)
The order of fields here really doesn't matter (has no real effect), but
if you do drop 'file' to `OPTIONAL` I'd prefer you move it last for
consistency.
> + REQUIRED(identifier, MDStringField, );
I'm pretty sure you want `/* AllowEmpty */ false` here. An empty string
will fail your Verifier check.
> + PARSE_MD_FIELDS();
> +#undef VISIT_MD_FIELDS
> +
> + Result = GET_OR_DISTINCT(DIExternalTypeRef,
> + (Context, tag.Val, file.Val, identifier.Val));
> + return false;
> +}
> +
> /// ParseDIFileType:
> /// ::= !DIFileType(filename: "path/to/file", directory: "/path/to/dir")
> bool LLParser::ParseDIFile(MDNode *&Result, bool IsDistinct) {
[skipping ahead]
> Index: lib/IR/AsmWriter.cpp
> ===================================================================
> --- lib/IR/AsmWriter.cpp
> +++ lib/IR/AsmWriter.cpp
> @@ -1591,6 +1591,18 @@
> Out << ")";
> }
>
> +static void writeDIExternalTypeRef(raw_ostream &Out,
> + const DIExternalTypeRef *N,
> + TypePrinting *TypePrinter,
> + SlotTracker *Machine, const Module *Context) {
> + Out << "!DIExternalTypeRef(";
> + MDFieldPrinter Printer(Out, TypePrinter, Machine, Context);
> + Printer.printTag(N);
> + Printer.printMetadata("file", N->getFile());
> + Printer.printString("identifier", N->getIdentifier());
Would putting 'identifier' before 'file' make the output more
check-able? If you allow null 'file' I strongly recommend swapping
order. If not, then you probably know better than I do, but my
intuition suggests that in absence of a proper name the `identifier`
might go there.
Also, since 'file' and 'identifier' are required fields, you should pass
`/* ShouldSkipEmpty */ false`. There's no benefit for working code in
skipping them, since they should never be null (and can't be parsed that
way anyway), and this will make `dump()` output more clear.
> + Out << ")";
> +}
> +
> static void writeDIFile(raw_ostream &Out, const DIFile *N, TypePrinting *,
> SlotTracker *, const Module *) {
> Out << "!DIFile(";
> Index: lib/IR/DIBuilder.cpp
> ===================================================================
> --- lib/IR/DIBuilder.cpp
> +++ lib/IR/DIBuilder.cpp
> @@ -425,6 +425,11 @@
> return DISubroutineType::get(VMContext, Flags, ParameterTypes);
> }
>
> +DIExternalTypeRef *DIBuilder::createExternalTypeRef(unsigned Tag, DIFile *File,
> + StringRef Identifier) {
> + return DIExternalTypeRef::get(VMContext, Tag, File, Identifier);
> +}
> +
> DICompositeType *DIBuilder::createEnumerationType(
> DIScope *Scope, StringRef Name, DIFile *File, unsigned LineNumber,
> uint64_t SizeInBits, uint64_t AlignInBits, DINodeArray Elements,
> Index: lib/IR/DebugInfoMetadata.cpp
> ===================================================================
> --- lib/IR/DebugInfoMetadata.cpp
> +++ lib/IR/DebugInfoMetadata.cpp
> @@ -295,6 +295,18 @@
> DEFINE_GETIMPL_STORE(DISubroutineType, (Flags), Ops);
> }
>
> +DIExternalTypeRef *DIExternalTypeRef::getImpl(LLVMContext &Context,
> + unsigned Tag,
> + Metadata *File,
> + MDString *Identifier,
> + StorageType Storage,
> + bool ShouldCreate) {
> + DEFINE_GETIMPL_LOOKUP(DIExternalTypeRef, (Tag, File, getString(Identifier)));
> + Metadata *Ops[] = {File, nullptr, nullptr, nullptr,
> + nullptr, nullptr, nullptr, Identifier};
Gosh, all these `nullptr`s are silly. I'll fix this eventually.
> + DEFINE_GETIMPL_STORE(DIExternalTypeRef, (Tag), Ops);
> +}
> +
> DIFile *DIFile::getImpl(LLVMContext &Context, MDString *Filename,
> MDString *Directory, StorageType Storage,
> bool ShouldCreate) {
> Index: lib/IR/LLVMContextImpl.h
> ===================================================================
> --- lib/IR/LLVMContextImpl.h
> +++ lib/IR/LLVMContextImpl.h
> @@ -443,6 +443,24 @@
> unsigned getHashValue() const { return hash_combine(Flags, TypeArray); }
> };
>
> +template <> struct MDNodeKeyImpl<DIExternalTypeRef> {
> + unsigned Tag;
> + Metadata *File;
> + StringRef Identifier;
> +
> + MDNodeKeyImpl(unsigned Tag, Metadata *File, StringRef Identifier)
> + : Tag(Tag), File(File), Identifier(Identifier) {}
> + MDNodeKeyImpl(const DIExternalTypeRef *N)
> + : Tag(N->getTag()), File(N->getRawFile()),
> + Identifier(N->getIdentifier()) {}
> +
> + bool isKeyOf(const DIExternalTypeRef *RHS) const {
> + return Tag == RHS->getTag() && File == RHS->getRawFile() &&
> + Identifier == RHS->getIdentifier();
> + }
> + unsigned getHashValue() const { return hash_combine(Tag, File, Identifier); }
> +};
> +
> template <> struct MDNodeKeyImpl<DIFile> {
> StringRef Filename;
> StringRef Directory;
> Index: lib/IR/Verifier.cpp
> ===================================================================
> --- lib/IR/Verifier.cpp
> +++ lib/IR/Verifier.cpp
> @@ -880,6 +880,10 @@
> &N);
> }
>
> +void Verifier::visitDIExternalTypeRef(const DIExternalTypeRef &N) {
> + Assert(!N.getIdentifier().empty(), "invalid identifier", &N);
Depending on what you do above, you may want to check the 'file' field
for null.
Also, do you want to restrict 'tag' in any way? Maybe at least require
that it's non-zero? (I'm not sure what makes sense.)
> +}
> +
> void Verifier::visitDIFile(const DIFile &N) {
> Assert(N.getTag() == dwarf::DW_TAG_file_type, "invalid tag", &N);
> }
> Index: test/Assembler/DIExternalTypeRef.ll
> ===================================================================
> --- /dev/null
> +++ test/Assembler/DIExternalTypeRef.ll
> @@ -0,0 +1,11 @@
> +; RUN: llvm-as < %s | llvm-dis | llvm-as | llvm-dis | FileCheck %s
> +; RUN: verify-uselistorder %s
> +
> +; CHECK: !named = !{!0, !1}
> +!named = !{!0, !1}
> +
> +!0 = !DIFile(filename: "foo", directory: "/dir")
> +
> +; CHECK: !1 = !DIExternalTypeRef(tag: DW_TAG_structure_type, file: !0, identifier: "c:objc(cs)Foo")
> +!1 = !DIExternalTypeRef(tag: DW_TAG_structure_type, file: !0, identifier: "c:objc(cs)Foo")
> +
I think there's an extra blank line at the end of the file here.
Usually I add tests -- spelled something like
"invalid-diwhatever-missing-file.ll" -- to check all the error messages
I'm expecting from the parser. It'd be good to have one for each
`REQUIRED` field (where you list the *other* required fields but leave
the tested one out, and confirm you get the right error message) and one
for each `/* AllowNull */ false` or equivalent.
More information about the llvm-commits
mailing list