[PATCH] D63521: Teach the symbolizer lib symbolize objects directly.

Thu Jun 27 02:26:43 PDT 2019

jhenderson added a comment.

In D63521#1560218 <https://reviews.llvm.org/D63521#1560218>, @ychen wrote:

> I don't think it's a loss of functionality versus the previous version which creates `DWARFContext ` even for COFF object (not work obviously). This version still does not work for COFF archive but looks a little bit more reasonable. IMHO, we should have no or all support for COFF archive, not partial support. I could either
>
> 1. Land the previous version (with the additional check for COFF so users are notified of the lack of support) and revisit COFF archive support later.
> 2. Support COFF archive file in this patch with the necessary tests.
>
>   I would prefer the first choice since one patch for each makes more sense. @MaskRay @jhenderson thoughts?

If we don't support COFF archives already at all (i.e. they don't work under any situation), then I don't think there's a problem that needs solving here, but we should definitely not make things worse. If archives containing COFF with PDBs work as well as before, then I think this is fine. If they no longer work, that's a problem and you should go back to the old behaviour. Beyond that, I don't think you need to fix the world in this patch. You've fixed the problem for DWARF-based debug information, and further improvements can be separate patches. Perhaps best would be to file a separate bug to track the additional cases that are unfixed by this?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63521/new/

https://reviews.llvm.org/D63521