[PATCH] D114830: [ELF] Hint -z nostart-stop-gc for __start_ undefined references

Fri Dec 3 20:50:01 PST 2021

MaskRay added a comment.

In D114830#3168986 <https://reviews.llvm.org/D114830#3168986>, @dblaikie wrote:

> In D114830#3168885 <https://reviews.llvm.org/D114830#3168885>, @smeenai wrote:
>
>> In D114830#3168865 <https://reviews.llvm.org/D114830#3168865>, @dblaikie wrote:
>>
>>> Generally once a patch is sent for review it should remain in review until approved, rather than committed without approval - so this patch should probably be reverted until approved?
>>
>> Hmm, is that standard convention?
>
> Generally - not sure if it's well documented, but I can probably find a few instances of this being sited over the years by folks other than myself.
>
>> I've observed a bunch of cases where contributors put up a review as an FYI or to get some additional thoughts, but then commit if there's no feedback or objections, for patches where they would have otherwise been comfortable committing directly for post-commit review.
>
> Yeah, there's some wriggle room, especially for code owners (like @maskray in this case) who might be seeking feedback but not feel like they're blocked on receiving that feedback. In that case I think the best/necessary thing to do is to document that ahead of time in the initial review - that the review is seeking feedback, but does not require it to proceed. That way it doesn't end up looking like the initial review required feedback, but the patch was then committed without feedback because the author wasn't willing to wait for folks to give that feedback. (that's the main reason for this rule: To ensure reviewers don't feel rushed/authors don't use short timelines as a way to push things through without the necessary review)

Ah, ok. Rushed on this because the combination of (a) the the code change would be useful for 13.0.1 and the code was the final form and the documentation page can evolve separately as I mentioned..
(b) this was (clearly as I think) what the release manager and some folks wanted.
(I was on the plane for many hours today. Pushed this so that there was a commit id with which the release manager knew what to cherry pick)

In D114830#3169336 <https://reviews.llvm.org/D114830#3169336>, @peter.smith wrote:

> I'm happy to approve in post; my apologies for not getting to it in time. Regardless of the opinion on the default value of start-stop-gc this particular change adds a useful warning and documentation.

Thanks!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114830