Page MenuHomePhabricator

[DWARFUnit] Don't assume basic types.
ClosedPublic

Authored by davide on Jan 3 2019, 3:23 PM.

Details

Event Timeline

davide created this revision.Jan 3 2019, 3:23 PM

+ llvm-commits.

davide updated this revision to Diff 180162.Jan 3 2019, 3:27 PM

update tests

JDevlieghere accepted this revision.Jan 3 2019, 3:29 PM

Can we further reduce this test case? If not this LGTM.

This revision is now accepted and ready to land.Jan 3 2019, 3:29 PM

Is the problem that it is a SubroutineType, but not a function *pointer*?
Generally, if the Verifier accepts it we ought to not crash, so this seems to be okay, but it might be worth asking whether this is really what you want your frontend to be creating since subroutinetypes don't have a size, etc.

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
472

Why is false the right answer here?

davide marked an inline comment as done.Jan 3 2019, 3:49 PM

Yes, the problem is that this is a Subroutine type. We might as well reject this altogether in the verifier, what do you think?

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
472

To be quite honest, I think it's irrelevant (in the sense that it doesn't matter what we return here).

davide added a comment.Jan 3 2019, 3:57 PM

Actually, on a second thought, this seems perfectly legit (although not really something useful to emit), so I'm leaning towards leaving the patch as is.
We can discuss whether true or false is the right answer, I don't have a preference there. I simply have hard time imagining what's "right" to do for a subroutine.

Is the problem that it is a SubroutineType, but not a function *pointer*?
Generally, if the Verifier accepts it we ought to not crash, so this seems to be okay, but it might be worth asking whether this is really what you want your frontend to be creating since subroutinetypes don't have a size, etc.

I'm the author of the frontend that is generating this code. Thanks for the explanation of the problem. Based on this I tried similar code with clang:

static void another(void) {}

int main() {
    void (*x)(void) = another;
    return 0;
}

To find out how it represented the debug info:

  call void @llvm.dbg.declare(metadata void ()** %2, metadata !11, metadata !DIExpression()), !dbg !15
!11 = !DILocalVariable(name: "x", scope: !7, file: !1, line: 4, type: !12)
!12 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !13, size: 64)
!13 = !DISubroutineType(types: !14)

So it looks like I should modify the frontend to represent this as a DerivedType with a baseType=SubroutineType. Is that right?

For what it's worth, I would consider this issue resolved if the verifier would reject the code. Although I will note that this assertion is not tripped in 7.0.0 for the same code.

andrewrk added inline comments.Jan 4 2019, 12:59 PM
llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
472

It seems to me that the question "is a subroutine an unsigned type" is a nonsensical question, and should be an assertion error (as it is in status quo trunk).

Or otherwise if a subroutine can be treated implicitly as a pointer, then it should have the same answer that a pointer would have, which is true.

So it looks like I should modify the frontend to represent this as a DerivedType with a baseType=SubroutineType. Is that right?

A DerivedType that's a pointer, yes. If that accurately describes your source.

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
472

I agree that asking whether a subroutine type is an unsigned type makes no sense. All contexts where we call isUnsignedDIType() are about emitting constant values. Why are we asking about a subroutine type?

llvm/test/Assembler/DIAsmunsigned.ll
2

This looks like Darwin naming, you probably want an explicit triple on the RUN line.

davide marked an inline comment as done.Jan 4 2019, 3:36 PM
davide added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
472

Is there anything that makes you think we should treat this implicitly as a pointer? I think this a very strong assumption to make in general.

davide marked an inline comment as done.Jan 4 2019, 3:39 PM
davide added inline comments.
llvm/test/Assembler/DIAsmunsigned.ll
2

Correct, I'll relax that.

andrewrk added inline comments.Jan 4 2019, 3:39 PM
llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
472

If you're asking for my opinion on this issue, what I think should happen is:

  • No change to DwarfUnit.cpp
  • Verifier gains support to reject the example IR as invalid

Is the problem that it is a SubroutineType, but not a function *pointer*?
Generally, if the Verifier accepts it we ought to not crash, so this seems to be okay, but it might be worth asking whether this is really what you want your frontend to be creating since subroutinetypes don't have a size, etc.

I'm the author of the frontend that is generating this code. Thanks for the explanation of the problem. Based on this I tried similar code with clang:

So it looks like I should modify the frontend to represent this as a DerivedType with a baseType=SubroutineType. Is that right?

Assuming that what your frontend produces is actually a function pointer or reference (and not the function *type*), yes.

For what it's worth, I would consider this issue resolved if the verifier would reject the code. Although I will note that this assertion is not tripped in 7.0.0 for the same code.

Correct. Any input that the Verifier accepts must be accepted by LLVM. We can fix this by either making the Verifier stricter (usually preferred), or by fixing LLVM.

davide closed this revision.Jan 7 2019, 2:14 PM