Open amtoine opened 1 year ago
actually, i think i found the real bug here :smirk:
the generation of my compiler looks ok, the only thing missing was...
type
field in the DISubprogram
debug node for the foo.__main
function :scream:here is the diff to apply to the above foo.ll
diff --git a/foo.ll b/foo.ll
index c9dddce..e3e348e 100644
--- a/foo.ll
+++ b/foo.ll
@@ -29,13 +29,15 @@ target triple = "x86_64-unknown-linux-gnu"
splitDebugInlining: false,
nameTableKind: None
)
-;!9 = !DISubroutineType()
+!100 = !{!3}
+!101 = !DISubroutineType(types: !100)
!10 = distinct !DISubprogram(
name: "main",
scope: !0,
file: !0,
line: 3,
+ type: !101,
scopeLine: 3,
flags: DIFlagPrototyped,
spFlags: DISPFlagDefinition,
and i get no errors on
> clang-15 foo.main.ll foo.ll -o foo.elf
and the result i wanted with gdb
, that is
> gdb --silent --ex "list" --ex "quit" foo.elf
Reading symbols from foo.elf...
1 MODULE foo;
2 VAR k: INTEGER;
3 BEGIN
4 k := 0
5 END foo.
hurray :tada: :star_struck:
@llvm/issue-subscribers-debuginfo
Yeah, looks like the LLVM IR verifier could be extended to document/test this property to give a more reliable/understandable failure.
Yeah, looks like the LLVM IR verifier could be extended to document/test this property to give a more reliable/understandable failure.
that would be awesome :star_struck:
for instance, when i remove a mandatory field, e.g. the file
in DICompileUnit
, clang
screams
> clang-15 foo.main.ll foo.ll -o foo.elf
foo.ll:37:1: error: missing required field 'file'
)
^
1 error generated.
this is nice and clear :yum:
@llvm/issue-subscribers-good-first-issue
Yeah - would probably be fairly easy to implement - though perhaps there's some hidden complications (like maybe it's common not to have the type on some DISubprograms, but not others?) - someone should give it a go and see what happens :)
Yeah - would probably be fairly easy to implement - though perhaps there's some hidden complications (like maybe it's common not to have the type on some DISubprograms, but not others?) - someone should give it a go and see what happens :)
the
type
field ofDISubprogram
is a mirror of the signature of the function whose "scope" will be this veryDISuprogramm
, right? does it make sense not having the signature of the function? :open_mouth:
i'm faaar from an expert, so i might very likely miss something here :wink:
You're feeding textual IR to clang (.ll file, not a .bc file)? I'd put the blame on the IR parser, in that case; it should have opinions about what fields are required or optional, and reject it immediately.
You're feeding textual IR to clang (.ll file, not a .bc file)?
yes it is automatically generated plain text IR so
.ll
plain text files instead of bitcode.bc
files :+1:I'd put the blame on the IR parser, in that case; it should have opinions about what fields are required or optional, and reject it immediately.
would be sensible :relieved:
On the plus side, that parser (at least as far as the debug-info metadata is concerned) is not too hard to navigate. I'd still consider it a good first issue if you wanted to tackle it. Probably you would rather get back to your own project, that's fair too.
@pogo59 that's interesting :thinking:
i'll get back to my project for the time being, but i'm quite interested in having a look at that :yum:
Looks like this has been quite since end of Dec. Assigning to @epilk, let me know if someone else is looking into this.
Setting the type field to REQUIRED in LLParser.cpp causes ~150 lit test failures, but I think it shouldn't be too bad to update them programmatically. Should we start enforcing this property? I guess the alternative is to allow type to be optional/nullable in DISubprogram, it seems like the DWARF emitter can tolerate having this field missing (after guarding the place where it's currently crashing with an if (SP->getType() != nullptr)
. I'm totally new to debug info, so I'm not sure what the better option is!
I guess maybe what I was thinking/alluding to in https://github.com/llvm/llvm-project/issues/59471#issuecomment-1349056404 was that possibly in some cases we legitimately produce DISubprograms without a type (maybe unprototyped functions in C?). If that were the case, maybe it'd show up in clang's test cases, rather than LLVM's - if we ran the verifier on clang's generated IR, which I believe we don't generally/as part of the normal flow of things.
So it might be worth tyring to see if there's a way to turn on the IR verifier for clang's IRGen pipeline and run check-clang and see if anything shows up?
Looking at only the lit test failures in https://github.com/llvm/llvm-project/issues/59471#issuecomment-1824971184 - checking if any of those are fully generated from clang, or possibly hand or machine reduced? if they're all hand/machine reduced, they might not be representative, but if some are full IR produced by clang, with comments describing the original source - maybe check if Clang still produces IR that looks like that & then we might know more about when this situation is normal and try to conclude what's different about the normal/accepted cases, and the ones you came across? It might be that a null type is only invalid in certain situations - and maybe we could catch those in the verifier without breaking many llvm tests, and without breaking clang.
Ok, thanks! Running check-clang
with a verifier that checks for null type fields causes no issues (looks like the verifier is run by default). I noticed a few failing lit testcases that seem to be clang output (1, 2), but I can't seem to convince (even old) versions of clang to ever generate a null type, so my best guess is that these tests are slightly hand-reduced. Unprototyped C functions still have a DISubroutineType. So it seems like the only place null type fields are ever actually generated are in lit test cases. ISTM that we could get away with enforcing non-null types.
I guess the next question is should we? It seems the DWARF writer can almost handle a null type, and the DWARF5 standard says the DW_AT_type attribute is only required for non-void returning subprograms (DWARF5 3.3.2). I'm happy to implement which ever, I guess I'm leaning towards requiring the type to be present since that seems to be the LLVM representation, I just don't have a good justification for that! Do you have a stronger opinion either way?
1: https://github.com/llvm/llvm-project/blob/main/llvm/test/Transforms/WholeProgramDevirt/devirt-single-impl.ll#L43 2: https://github.com/llvm/llvm-project/blob/main/llvm/test/DebugInfo/MIR/X86/debug-call-site-param.mir#L111
the DWARF5 standard says the DW_AT_type attribute is only required for non-void returning subprograms (DWARF5 3.3.2).
Yeah, that has always been the case with regard to the DWARF description.
As far as the IR goes, I'd wonder how the various non-Clang front-ends to LLVM (Rust, Julia, flang, whatever) describe no-return-value functions; do they emit a type that's null, or not provide a type at all? (Does DIBuilder allow you not to provide a type? If DIBuilder makes you provide a type, then the textual IR should also require a type.)
The difference is that the way we encode the debug info in the IR is that the type
parameter of the DISubprogram
is not the return type, but the whole function type. I think we carry a bunch of other stuff on that function type too - probably things like rvalue ref/lvalue overloading, const for const member functions, etc. (hmm, seems we do carry const
there, but we redundantly encode rvalue
-ness between the function type and a flag on the DISubprogram
)
There's probably some tech debt that could be paid down in normalizing these things - but for now, I guess, making sure type
is provided, and that it's a DISubroutineType, would probably be good.
The difference is that the way we encode the debug info in the IR is that the type parameter of the DISubprogram is not the return type, but the whole function type.
Aha. In that case I agree it's reasonable to consider a null type
to be broken metadata, and make it mandatory.
the
type
field ofDISubprogram
is a mirror of the signature of the function whose "scope" will be this veryDISuprogramm
, right? does it make sense not having the signature of the function?
Only place I could think of maybe we wouldn't have a subroutine type for a subprogram would be an unprototyped C function, but even then we still emit a subroutine type and use a flag (well, the absence of the prototyped
flag) to specify that a function lacks a prototype. So, yeah, looking like subprograms should always have an associated subroutine type.
(I'm still not sure that clang does verify the IR when it's emitting it, but if you're pretty sure it does/that you've checked a fair amount of IR and nothing fails this verification - I'm OK with moving forward with making "subprograms must have types and they must be a subroutine type" a verifier check/failure)
hello there :wave: :yum:
i've followed your contributing guidelines to the "Crashing bugs" section, however i cannot go further in the bug report flow, i get an error which is not mentionned in the section :open_mouth: when i add
-emit-llvm
as given in the section, i geti've tried to be as concise as possible, given the sizes of the files i manipulate, with simple examples, but also as complete as possible... i hope this is not too large nor to small :relieved: please fell free to ask anything :wink:
a tiny bit of context
i am writing a compiler for the Oberon language :+1: this is a work in progres, but it is already able to take some
oberon
input, translate it to llvm IR and finally useclang
to optimize the IR and generate some machine code binary for the host target :ok_hand:EDIT
i've changed the example for an even simpler one
my issue
i have the following
oberon
source filethe compiler turns it into the two following IR files, thanks to a new
-g
option i'm currently working on! i.e. the command is here./oberon -g compile foo.obn
and
NOTE: all of the above is generated dynamically by the compiler :warning:
finally, the error i get is the following
additional context
compiling without debugging information
a simpler
./oberon compile foo.obn
gives meand
and
works just fine :yum:
clang
installationi've installed the latest available precompiled binary from here, inside my
~/.local/share/
directory and the version is:ok_hand: