Make users aware what to do with ld.lld 13.0.0 / GNU ld<2015-10 --gc-sections
This won't diagnose the case where the actual contents of the C-named section get dropped, which can only be caught at runtime. For example, our application of encapsulation symbols for library merging was relying on C-named sections being retained; the behavior change in LLD 13 didn't result in any link-time errors, but it caused run-time crashes because all the JNI_OnLoads were dropped. (It was straightforward for us to add __attribute__((retain)) because of how our builds are set up, so it wasn't a problem for us.) This is still a good change; just pointing out that it doesn't capture all possible cases which might change program behavior.
Hmm, is that standard convention? 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.
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)
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.
As an aside Arm's proprietary linker introduced a "weakly live" value for sections referenced by the equivalent of __start and __stop. The linker would follow relocations from a "weakly live" section to see if it could reach an existing "live" section from the same object file, where "live" is the result of classic garbage collection assuming --start-stop-gc . This heuristic kind of worked for what we needed it do, which was to permit all exception tables (reachable via start and stop) to be thrown away if no object called throw. We didn't have any real rigorous theory to back up the heuristic though other than it was conservative enough to keep what we need to keep and gave good enough GC results in most cases. I'd not recommend LLD follow that though as it was a heuristic tuned to a system library.
This won't diagnose the case when the __start_ reference is weak.
Among the few Debian Code Search examples, I cannot find undefined weak examples.
I know that Clang InstrProfiling uses undefined weak. Such examples are just rare.
For such cases, we have to rely on the developer to understand the problem. (The section just doesn't exist, so presumably not too difficult to troubleshoot.)
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)