[Lldb-commits] [PATCH] D128268: [lldb] Fix reading i686-windows executables with GNU environment

Sun Jun 26 14:00:53 PDT 2022

mstorsjo added a comment.

In D128268#3608197 <https://reviews.llvm.org/D128268#3608197>, @labath wrote:

> In D128268#3604555 <https://reviews.llvm.org/D128268#3604555>, @mstorsjo wrote:
>
>> I found that this duality was introduced in 5e6f45201f0b62c1e7a24fc396f3ea6e10dc880d / D7120 <https://reviews.llvm.org/D7120> and ad587ae4ca143d388c0ec4ef2faa1b5eddedbf67 / D4658 <https://reviews.llvm.org/D4658> (CC @zturner), what do you make out of the reasonings in those commits?
>
> The first patch seems like it's just working around some mismatches in the windows dynamic loader plugin. I think a better approach would be to have the dynamic loader claim both architectures, though I don't think that is necessary if we're just consistent about what we use. I don't see anything wrong with the second patch (the darwin platform does something similar for arm architectures, even though I'm not convinced it's necessary (the reason it's necessary for darwin is because there they actually make a distinction between armv6XX and armv7YY and treat those as different architectures).

The odd thing about the second one is the added hardcoded `AddArch(ArchSpec("i686-pc-windows"));` and `AddArch(ArchSpec("i386-pc-windows"));` at the top of `PlatformWindows.cpp`.

Anyway, it does seem to work fine in my quick test to get rid of this duality, so I'll post a patch that does that.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128268