[PATCH] D118072: [X86] [CodeView] Add codeview mappings for registers ST0-ST7

Tue Jan 25 00:05:58 PST 2022

mstorsjo added a comment.

In D118072#3267788 <https://reviews.llvm.org/D118072#3267788>, @aganea wrote:

> In D118072#3267627 <https://reviews.llvm.org/D118072#3267627>, @mstorsjo wrote:
>
>> In D118072#3267462 <https://reviews.llvm.org/D118072#3267462>, @aganea wrote:
>>
>>> The feature can still be enabled with `-mllvm -experimental-debug-variable-locations=true`, so D116821 <https://reviews.llvm.org/D116821> wasn't completely reverted.
>>>  (l
>>> Why not adding your initial bug repro as a test, along with the flags above on the cmd line? It also looks like that prior mappings were added without test coverage.
>>
>> Ok, I made such a testcase. I'm a bit afraid that it might be a little brittle in the long run though...
>
> I was simply surprised that a such simple testcase wasn't catched elsewhere. Could you please explain 'brittle'? Do you think the outcome of this test isn't stable in the long run?
> In any case LGTM.
> I'll let the choice (to add a test or not) to your discretion ;-) I felt that it could make you loose time again later down the line...

Brittle, in the sense that it depends on all the internal machinery, whether this input piece of code actually produces the debug info we want to test. (Up until recently, it didn't, and now it does with a flag only.) I mean, it would be more robust if I'd take the intermediate output from a stage when everything has been settled about what debug info to produce, and then only produce codeview debug info out of it. I.e. at a point where the input specifically says "debug info for `X86::ST0`". But then on the other hand, that test would pretty much just test whether `X86::ST0` is include in the table too.

So I guess this is fine - and in case the test causes problems later, we can reconsider if it has to be kept or not.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118072