Before this patch, we use (uint32_t)AbbrCode - (uint32_t)FirstAbbrCode to index the abbreviation. It's impossible for we to use the preceeding abbreviation of the previous one (e.g., if the previous DIE's AbbrCode is 2, we are unable to use the abbreviation with index 1). In this patch, we use AbbrCode to index the abbreviation directly.
Probably "ignores the contents of entries with abbrev code 0". (I wouldn't use the full term "abbreviation" as I think it's not particularly common usage in this context).
I'd also move this case later in the sequence (possibly the last one), since we should allow a non-zero value to be the "First" abbrev.
using the ... -> when the abbrev code is specified.
I think the value 2 is unimportant here.
using the ... -> when the abbrev code is specified to be lower than the first abbrev.
There's a bit of tension here between the AbbrOffset and the behaviour. Since the AbbrOffset should point to the start of the table containing the corresponding abbrevs, it's a little unclear what the behaviour should be if the offset is nonsensical. It also prevents us having multiple abbrev tables as it stands.
I think it's okay to add this test case for now, but I might suggest a change for the future, to split the abbrev section into distinct tables which can be referenced by some ID from the .debug_info. Different .debug_info tables can refer to the same or different abbrev tables that way. The offset would be auto-calculated to match, but could be overridden to a different value (the ID could still be used to actually identify how to write the values).
Doesn't seem this assert is helpful. AbbrevDecls is std::vector<Abbrev>,
Probably you should add a test case and do the following for now:
if (AbbrCode > DebugInfo.AbbrevDecls.size()) report_fatal_error(...)
Since you are touching this line, could you convert the auto to an actual type?
Sorry, I haven't tested a report_fatal_error() before. What will be checked? The dumped stack trace? or something else? Can I leave a "TODO" here and let this function return Error in a follow-up patch?
Yeah, the current implementation of the .debug_abbrev section doesn't support multiple tables. I deleted this test. I will it once generating multiple abbreviation tables is supported.
Typically, you wouldn't test a report_fatal_error as it shouldn't be possible to reach that bit of code. It's like an assert, but works without asserts being enabled too.
Just the fact of crash of the test case. In the follow-up you could fix the crash then.
RUN: llvm-mc -filetype=obj -triple aarch64-windows %s -o - \
I do not mind leaving the test case for a follow-up. It was just important not to use an assert here.
Thanks! I added one more test case for it :-)
I'd change "Report" to "Handle", since the error is now being reported (although not in the best way).