[PATCH] D125783: [llvm-debuginfo-analyzer] 08 - ELF Reader

Mon Oct 31 01:49:37 PDT 2022

CarlosAlbertoEnciso added a comment.

In D125783#3893003 <https://reviews.llvm.org/D125783#3893003>, @dblaikie wrote:

> A bit concerned about two things that happened here (though, apologies as I haven't otherwise been following the review):

Very much appreciated your input and I fully understand your concerns about code reviews and large/complex patches.

> 1. disabling tests - generally code is reviewed with the tests, disabling the tests renders the review approval non-standing because the code is now not covered by tests. Usually the solution is to revert the patch while addressing issues like this.

There are 2 specific cases that are failing:

- Memory leak detected by the sanitizers: `tools/llvm-debuginfo-analyzer/DWARF/pr-incorrect-logical-instructions.test`

Currently I am working on a fix which I hope to submit in the next couple of days.

- unexpected output: `tools/llvm-debuginfo-analyzer/DWARF/06-dwarf-full-logical-view.test`

The test fails on some ARM configurations and  i386 (32-bit) as reported by @mgorny.
>From an early analysis based on the error, the code associated with `--print=sizes` fails.
I can enable the test and change `--print=all` which is equivalent to `--print=elements,warnings,summary,sizes` to `elements,warnings,summary` and open a GitHub issue.

> 2. code review feedback (such as memory allocation handling) being pushed back to some much later stage isn't usually the way we do things if it's reasonable observations about how things are done - postcommit (or, it sounds like precommit) feedback is to be addressed "There is a strong expectation that authors respond promptly to post-commit feedback and address it." (perhaps you could link to the other discussion on the raw `new` usage issue? Might help provide explanation for why it's not already addressed?)

We are fully aware of the issues related with the current memoy allocation and changing it to use smart pointers is a priority.
Based on your suggestion, I can create a GitHub issue to include all the information and add that link to each of the entries in the reviews that refer to the memory handling.

> I understand this is a complex/large patch series, but these are the sort of reasons they are generally discourageed - because it's expensive for both reviewers and authors to do quality review when lots of code is already written on top of the patches being reviewed (you get that pushback of "but if I fix this, all the other patches are broken/need fixes, and that's expensive").

Thanks for your understanding on the nature of the patches.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125783