[PATCH] D19236: Add DITypeIndex for CodeView and emit it for locals

Wed Apr 27 15:30:21 PDT 2016

On Wed, Apr 27, 2016 at 12:47 PM, Reid Kleckner via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> rnk marked 3 inline comments as done.
> rnk added a comment.
>
> In http://reviews.llvm.org/D19236#414087, @dblaikie wrote:
>
> > Generally looks good - should there be more test coverage, though? At
> least testing one case of a type other than int being generated into the
> code view? (to demonstrate the new codepath, etc - it looks like the only
> things testing the CodeView type reference emission are testing the old
> "everything is int" implementation?)
>
>
> I changed local-variable.ll to have some 'unsigned' variables.
>
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:712-713
> @@ -711,1 +711,4 @@
>
> +  // Pull the type index out of the type field. If the frontend did not
> provide
> +  // a DITypeIndex, emit zero for "no type".
> +  unsigned TI = unsigned(SimpleTypeKind::None);
> ----------------
> dblaikie wrote:
> > Is this a supported case? Sounds like if that happens it's corrupt debug
> info, no? (catch this in the verifier & then just assert that it'll always
> be present here?)
> IMO it is useful to allow users to emit codeview for IR that was generated
> with DWARF in mind. You'll still get line tables etc. Rejecting it in the
> verifier and asserting here would make that pretty hard.
>

I'm not sure that's a middle ground we really want to sign up to
support/figure out, though.

>
> ================
> Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:714
> @@ +713,3 @@
> +  // a DITypeIndex, emit zero for "no type".
> +  unsigned TI = unsigned(SimpleTypeKind::None);
> +  if (auto *DTI = dyn_cast_or_null<DITypeIndex>(Var.DIVar->getType()))
> ----------------
> dblaikie wrote:
> > static_cast rather than C style cast?
> You don't like C++ conversion-style casts? :)
>

I'd be hard pressed to actually demonstrate the relative use of c-style
versus C++ casts across the codebase (only because it's really hard to
search for the C ones), but I'd guess we're pretty consistent about using
the C++ style casts & consider them safer & more self documenting (because
they do a strict subset).

At least I'm pretty sure if we do use C-style casts we write them this way
"(T)t" rather than in the function-style syntax, fairly regularly?

It also looks extra weird in an initialization, you could just write it as:
"unsigned TI(SimpleTypeKind::None)"; (or use auto, if the type's explicit
on the RHS anyway)

>
> ================
> Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:591-592
> @@ +590,4 @@
> +  DITypeRefArray FnArgs;
> +  if (DISubroutineType *FnTy =
> +          cast_or_null<DISubroutineType>(resolve(Sub->getType())))
> +    FnArgs = FnTy->getTypeArray();
> ----------------
> dblaikie wrote:
> > Under what conditions would this fail? Is there a test case for it?
> The verifier checks that this is a DISubroutineType, DITypeIndex, or null.
> 'resolve' internally casts to DIType, so if you attempt to generate DWARF
> from IR with type indices, you will crash here. You will also crash at
> every other 'resolve' call site, though.
>

I'm not quite following - I mean when does this conditional evaluate to
'false', not when does it crash. Does this ever evaluate to false? If the
type reference is a null pointer?

> Do you think we should apply the same logic that we have on the CV side
> and have 'resolve' return nullptr for a DITypeIndex?
>

No, I don't think so - (& I think we should probably be symmetric and make
it invalid the other way too - referring to the above discussion on the CV
side)

>
> ================
> Comment at: lib/IR/DebugInfoMetadata.cpp:639-640
> @@ +638,4 @@
> +  if (!Slot) {
> +    void *Mem = Context.pImpl->TypeAllocator.Allocate(sizeof(DITypeIndex),
> +
> alignof(DITypeIndex));
> +    Slot = new (Mem) DITypeIndex(Index);
> ----------------
> dblaikie wrote:
> > Is this the first use of TypeAllocator for something other than types?
> Should it be renamed, or at least a comment added? (or a different
> allocator used?)
> Oops, I assumed it was used for DI types, not LLVM IR types... Well, they
> have the same lifetimes. I'm going to update the doc comment to indicate
> that they also live here.
>

OK

>
> ================
> Comment at: test/Linker/ditypeindex.ll:33-36
> @@ +32,6 @@
> +; SECONDFILE: !5 = !DIGlobalVariable(name: "c", scope: !0, file: !1,
> line: 1, type: !DITypeIndex(u0x13), isLocal: false, isDefinition: true,
> variable: i64* @c)
> +; SECONDFILE: !6 = !{i32 2, !"CodeView", i32 1}
> +; SECONDFILE: !7 = !{i32 2, !"Debug Info Version", i32 3}
> +; SECONDFILE: !8 = !{i32 1, !"PIC Level", i32 2}
> +; SECONDFILE: !9 = !{!"clang version 3.9.0 "}
> +
> ----------------
> dblaikie wrote:
> > Presumably you don't need to check that all this other stuff roundtrips
> - just the indexes, right? Your change didn't have anything to do with
> those other bits.
> >
> > Oh, I see, these aren't CHECK lines, they're for mutating this file. -
> not sure if that's ideal/necessary. (& won't that mean this test "REQUIRES:
> shell"? Which is probably best avoided (I assume we generally try to avoid
> that, but don't know for sure), but should be added if you're going to keep
> the grep/sed/cat/etc)
> >
> > I'd just write it as a second file - at least that's what we've usually
> done, I think.
> Fine. Personally, I find it convenient to be able to see the whole test
> case all at once, but it is not a common style and pretty confusing.
>
> ================
> Comment at: unittests/IR/MetadataTest.cpp:83
> @@ +82,3 @@
> +  DITypeRef getSubroutineType() {
> +    return DITypeRef(
> +        DISubroutineType::getDistinct(Context, 0, getNode(nullptr)));
> ----------------
> dblaikie wrote:
> > Is this conversion implicit? (I think it is, but didn't entirely follow
> all the layers of macro goo, inheritance, etc)
> No, it is not. I drafted a change to make it implicit, but I think Duncan
> said he'd rather not do that.
>

OK

>
> ================
> Comment at: unittests/Transforms/Utils/Cloning.cpp:422
> @@ -422,3 +421,3 @@
>      DITypeRefArray ParamTypes = DBuilder.getOrCreateTypeArray(None);
> -    DISubroutineType *DFuncType =
> DBuilder.createSubroutineType(ParamTypes);
> +    DITypeRef DFuncType(DBuilder.createSubroutineType(ParamTypes));
>      auto *CU =
> ----------------
> dblaikie wrote:
> > Prefer copy init over direct init where both are valid:
> >
> >   DITypeRef DFuncType = DBuilder.createSubroutineType(ParamTypes);
> >
> > (this avoids any accidental explicit conversions from being performed)
> > (same comment on the previous instance of the same construct in this
> file)
> Both instances need explicit conversion, though.
>

Ah, my mistake - thanks!

>
>
> http://reviews.llvm.org/D19236
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160427/b8399bfd/attachment.html>