labath wrote: > Seems reasonable to me, given we end up here only for definition DIEs. (did > have couple of questions though) > > > My fix consists of always fetching type size information from DWARF (which > > so far existed as a fallback path). I'm not quite sure why this code was > > there in the first place (the code goes back to before the Great LLDB > > Reformat), but I don't believe it is necessary, as the type size (for types > > parsed from definition DIEs) is set exactly from this attribute (in > > ParseStructureLikeDIE). > > Was it possibly protecting against cases where we generate structure > definitions without a size attribute? (as you allude to in your test > comment)?
I can't disprove that, but I think it would be a very roundabout way of achieving that. To support the scenario of missing DW_AT_byte_size, I'd probably do something like set the size field to `max(offsetof(Struct, field)+sizeof(Struct::field) for field in Struct)`, or change the record layout builder to infer the size as well. If i had to guess, I'd say this was an attempt to avoid parsing the attributes of the record DIE (again) in the completion step. In a world where where the size was already parsed in the parsing step, I can see how accessing the parsed value could be considered faster (though probably not measurably) than going to DWARF. > Reporting back a `ExternalLayout::Size` of `0` back to the > `RecordLayoutBuilder` seems problematic. If `Alignment == 0`, the > `RecordLayoutBuilder` knows to infer it. But we don't have that for `Size`, > so it just takes it at face-value. I don't know if that's something we should > account for. Probably not? In principle I would say that we should, but I think this problem is in the same bucket as compiler putting a nonsensical (small) value in the DW_AT_byte_size field. In theory that should not crash lldb, but we're not likely to run into an actual compiler that does that. > Side-note, should we also check `DW_AT_bit_size` here? The DWARFv5 spec says: > > ``` > A structure type, union type or class type entry may have either a > DW_AT_byte_size or a DW_AT_bit_size attribute (see Section 2.21 on page 56), > whose value is the amount of storage needed to hold an instance of the > structure, > union or class type, including any padding. > ``` We could do that, but then we'd also need to do that in ParseStructureLikeDIE. And we may need to figure out what does it mean to occupy a non-whole number of bytes, so I'd say that's orthogonal to this. > > > The problem is that if the type object does not have this information > > cached, this request can trigger another (recursive) request to lay > > out/complete the type. This one, somewhat surprisingly, succeeds, but does > > that without the type layout information (because it hasn't been computed > > yet) > > That sounds worrying. Wonder if we do this anywhere else around the DWARF AST > parser. And is there any safeguarding against these recursive calls? We have a bunch of calls like that, but I don't think they're a problem, as they would just trigger a regular type completion, compute the dwarf-guided layout and then get the size from that. The problem is really specific to this place in the code, where we've already completed the clang type, but we haven't created the layout for it. https://github.com/llvm/llvm-project/pull/110648 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
