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

Fri Jun 24 07:29:16 PDT 2022

labath added a comment.

In D128268#3604555 <https://reviews.llvm.org/D128268#3604555>, @mstorsjo wrote:

> In D128268#3604081 <https://reviews.llvm.org/D128268#3604081>, @labath wrote:
>
>>> If we'd just set this to the baseline, i386, would that have any effect for how lldb e.g. is able to disassemble/interpret instructions that don't exist in the i386 baseline architecture?
>>
>> It should not have any effect (if it does, that's a separate fix). In the disassembler, we explicitly enable the latest instruction set, and I can't think of anything else that would be impacted by it.
>
> Thanks - I did some cursory testing with removing the extra i686 everywhere, and at least on a quick test, it seems to work just fine (and requires a minor adjustment to only one testcase).
>
> 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).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128268