[BOLT][DWARF] Add support for .debug_names

[ayermolo]

DWARF5 spec supports the .debug_names acceleration table. This is the formalized version of combination of gdb-index/pubnames/types. Added implementation of it to BOLT. It supports both monolothic and split dwarf, with and without Type Units. It does not include parent indices. This will be in followup PR. Unlike LLVM output this will put all the CUs and TUs into one Module.

[dcci]

Some initial run of comments.

[aaupov]

If the method computes DIE offset, it's better to keep the CU parameter a constant ref, and do the modifications outside of this method.

[ayermolo]

I removed const because DWARF5AcceleratorTable::addUnit invokes StrOffsetsWriter.initialize(Unit); which invokes DebugStrOffsetsWriter::initialize, which invokes Unit.getStringOffsetsTableContribution() that is non const. Because it invokes extractDIEsIfNeeded.

[aaupov]

I'm strongly in favor of keeping CU a const ref since semantically computeDIEOffset should not change the input CU (or DIE). Please make modifications to the CU outside of this method.

[ayermolo]

I am not making changes to CU. I am invoking none const APIs from it.

[aaupov]

Can you then add a const API so you don't have to drop const qualifier? As a separate diff.

[ayermolo]

Issue is

 const std::optional<StrOffsetsContributionDescriptor> &
  getStringOffsetsTableContribution() {
    extractDIEsIfNeeded(true /*CUDIeOnly*/);
    return StringOffsetsTableContribution;
  }

It parses debug information and sets StringOffsetsTableContribution first time it's called.

I can add

  const std::optional<StrOffsetsContributionDescriptor> &
  getStringOffsetsTableContribution() const {
    assert(StringOffsetsTableContribution);
    return StringOffsetsTableContribution;
  } 

But then implementation will rely on extractDIEsIfNeeded being invoked for this cu at some point. Probably will, but this seems flaky to me.

[ayermolo]

I tried with change on an internal service of reasonable size. No issues, but having basically the same API with quite different behavior based on const seems like just asking for trouble.

[aaupov]

I hope you understand why I'm raising this issue – that if the method is called computeDIEOffset, it's reasonable that CU is passed as a const ref.

Dropping the const qualifier is a symptom of a larger issue: that a parameter that's not supposed to be modified starts to be modified. I understand that it's not possible/practical to add a const-qualified method, that's ok. I suggested moving the modifications to CU/calling non-const stuff outside of this method – what do you think about that?
As a final resort, you can rename the method to computeDIEOffsetAndAddAccelTableEntry and drop const.

[ayermolo]

I see your point. The reason I put adding entries here to avoid traversing all these CUs/DIEs again and wasting resources.
I can get StringOffsetsTableContribution before the call, but I don't think it fundamentally addresses your concern. Why are we passing StrOffsetsContributionDescriptor into computeDIEOffset?

So I think the option you mentioned of renaming to something else would be the way to go. Maybe something more generic processAndFinalizeDIEs? In case down the road we add more stuff here. For example once ability to delete dies is added I think computing GDB Index will need to move out of main updatedebuginfo.

[dcci]

LGTM if Amir's happy with it.

[ayermolo]

Stats collected on internal binary, monolithic dwarf, with descent size .debug_names section (bolt generated)
[70] .debug_names PROGBITS 0000000000000000 1b03a2b06 877d092 00 0 0 1

base:
9:12.18 real, 642.61 user, 186.98 sys, 0 amem, 78477316 mmem
.debug_names before string offset fix:
11:02.32 real, 742.95 user, 188.14 sys, 0 amem, 82591356 mmem
.debug_names after string offset refactor (no longer using DebugStrOffsetsWriter)
9:42.25 real, 668.48 user, 188.72 sys, 0 amem, 82923700 mmem

[aaupov]

Thank you for working on debug_names support. LGTM but I'd wait for Maksim's comments if he has any.

[maksfb]

Any reason for creating DebugNames under Core and not under Rewrite?

[ayermolo]

Any reason for creating DebugNames under Core and not under Rewrite?

Any reason for creating DebugNames under Core and not under Rewrite?

Well it's not a re-writer it's a supporting data structure. I viewed it similar to DIEBuilder, DebugData

[maksfb]

Any reason for creating DebugNames under Core and not under Rewrite?

Well it's not a re-writer it's a supporting data structure. I viewed it similar to DIEBuilder, DebugData

We've discussed the file placement offline. We'll need a separate PR to move DebugNames out of \Core as Alexander ran into a circular dependency issue while trying to do so.