[PATCH] D148761: Emit the correct flags for the PROC CodeView Debug Symbol

Tue May 16 04:26:32 PDT 2023

aaron.ballman added a comment.

In D148761#4344803 <https://reviews.llvm.org/D148761#4344803>, @omjavaid wrote:

> In D148761#4343870 <https://reviews.llvm.org/D148761#4343870>, @aaron.ballman wrote:
>
>> In D148761#4343528 <https://reviews.llvm.org/D148761#4343528>, @omjavaid wrote:
>>
>>> I am the owner of lldb-aarch64-windows and i have verified that exactly this commit caused the test to fail. The builder was failing a few test earlier thats why this particular failure got masked.
>>
>> Did you verify that the failure was because the changes were incorrect? These changes are correcting a bug and it's expected that downstreams like lldb may have to react to that sort of thing breaking their tests. A revert ~10 days after the failure landed is not exactly a timely revert -- now all the downstreams that did react to this change will have to react again, which is why I'm curious what amount of analysis was done prior to reverting and what details you can share about the problem.
>
> HI AAron
> I do apologise for the inconvenience this may have caused. 
> We normally try to be proactive with reverts but this was delayed as I was holidays past 10 days.
> Regarding revert decision I believe the general community guideline is to revert any patches that cause a regression thats why I have reverted the change and I do agree with correctness of the patch.

Our revert policy is at: https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy and it does encourage timely reverts to regressions, but 1) this change doesn't appear to be a regression, 2) this revert was not timely. Correct, conforming changes to Clang or LLVM that break lldb are not a reason to revert the changes in Clang or LLVM. lldb is a downstream consumer of these tools and will sometimes need to react to changes that break their tests (Clang and LLVM developers will of course try not to be disruptive though!). Just something to keep in mind for next time, not the end of the world. :-)

In D148761#4345436 <https://reviews.llvm.org/D148761#4345436>, @omjavaid wrote:

> Hi Aaron,
>
> The problem seems to be appearing in lldb-test tool which in this case is being used to fetch symbol information. Specifically it fails to find function name inside a nested block with your patch applied. More investigation on LLDB side is needed to find the exact cause of this failure.
>
> You may re-land your changes as they are already approved and kindly mark the LLDB test as xfail.

Thank you for doing the investigation! @efriedma, can you re-land this for @dpaoliello?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148761