This LGTM!

Looks good to me.

Looks good, thanks!

• I guess a possible alternative would have been to set LargeCodeModel to true for kernel case? Regardless, I'd prefer being explicit to prevent confusion.
• I'm slightly worried about the default value being CodeModel::Small in case other parts in MCObjectFileInfo start relying on this, but it's definitely the most sensible.

Few nits

Thanks Paul! One nit, otherwise this LGTM.

This looks really cool :-)

Add comment to tests explaining why this works on Darwin.

Thanks for the review Stefan!

Updated to use the ReadContext. If the error message doesn't matter that much then I agree this is better.

In D50213#1187506, @JDevlieghere wrote:

Regarding the naming: I like PubnameKind because of the aforementioned consistency (we also have AccelTableKind. Can't we use an enum class to prevent collisions?

Regarding the naming: I like PubnameKind because of the aforementioned consistency (we also have AccelTableKind. Can't we use an enum class to prevent collisions?

In the differential you linked we agreed to have the API include an error code as an argument. That still leaves us the option to pass an inconvertibleErrorCode() as the first argument.

Landed alternative from D49679

• Address Dave's feedback

The consensus is that we won't support this, at least for now.

LGTM, Thanks for catching this!

I agree with Pavel, but I also don't mind having this test in the meantime. LGTM if you add a FIXME with the direction we want to go.

Third time's the charm!

Lang's suggestion was indeed what I had in mind. Thanks!

Please run the test through llvm-as | llvm-dis, this will fix the numbering of the DI nodes.

Can you add a test for the case where no Vals are provided? I remember something about that requiring an overload without the variadic template arguments.

In D49819#1175942, @aenewton wrote:

Thanks for the comments, Jonas. I grepped through the tests and saw that none of them were checking against the UnknownCode string that would have been produced. It looks like I'll need to create a bitcode file that contains the codes and create a new test based on that.

LGTM.

Thanks Adam, the change looks good but would need a test indeed. Currently the codes are not printed if they are not known, so maybe by adding them they are now printed in one of the existing tests? It would be fine to add a new test if we can't reuse an existing one.

Given these numbers I think we can land this patch as is and figure out the IDP stuff on the mailing list and/or a future patch. LGTM. Thanks Stefan!

• Address Dave's feedback

Address Pavel's feedback:

• Specialize std::reverse_iterator for llvm::DWARFDie as suggested by Pavel.

In D49679#1173122, @labath wrote:

Sorry I wrote the reply without looking at the patch in detail. I did not notice you demoted the iterator back to "forward". In case this is not a bidirectional operator, but a collection of two forward iterators that happen to iterate in the reverse order, the specializing std::reverse_iterator is not the right approach.

I have to admit I haven't thought about the specifics of how the reverse iterator would work until now. However, now that I think about it, I think it could be implemented as a pair of DWARFDie + bool flag. The trick would be that we store the element that we really dereference to, instead of its successor. The bool flag would be used denote the before_begin state, as we cannot represent rend() this way. The implementation would do something like:

operator++() {
  assert(!AtEnd && "Incrementing rend");
  DWARFDie D = Die.getPreviousSibling();
  if (D)
    Die = D;
  else
    AtEnd = true;
}

operator--() {
  if (AtEnd) {
    AtEnd = false;
    return;
  }
  Die = Die.getNextSibling();
  assert(!Die.isNULL() && "Decrementing rbegin");
}

Looks good to me but I'll defer to Greg and Dave who had actual feedback.

• Feedback Pavel

In D49679#1172143, @labath wrote:

What's the reason for not specializing std::reverse_iterator? AFAIK, that is officially permitted by the standard.

Other than the DIDumpOptions comment (which I think you might have missed) this LGTM.

• Ignore modules for now as discussed offline.
• Document this decision.

I've put up https://reviews.llvm.org/D49679 as an alternative to this patch. Personally I don't like the caveat that you can't store the DWARFDie the iterators point to, but on the other hand I don't like returning by value either. Please let me know which you prefer.

In D49594#1169809, @probinson wrote:

Is this because type units depend on COMDAT support? I had a vague idea that COFF also supports COMDAT.

Looks good to me, but I'll defer to Paul

Thanks, I meant to use isOSBinFormatELF but I think it got lost in an accidental undo. The test didn't capture that because it's using a linux target triple.

Please see PR38190 for more details.

I'm happy with this, LGTM!

LGTM. This is definitely much better!