This was (re)landed as rGff171acf8

Oops, this is just the diff from D71681 (which was committed as rG3a05c3969c18 and reverted), but not the whole patch.
I'll reland the whole thing together, but if @yurydelendik or @sunfish have comments I'm happy to hear them.

• Cleanup scope, update lld debuginfo test

Thanks, I'll commit this soon.

Ping.

• Fix NVPTX

Rebase

• suppress build warning

also cc @paolosev and @philip.pfaffe

This is ready for review now, please take a look.

• add doxygen comment

• finish update multi-return
• remove debug prints
• clean up, add comments
• clang-format

• update tests, add new test for dwarf local var
• start update multi-return test

• use getDwarfFrameBase on WebAssemblyFrameLowering
• Don't optimize live intervale for frame reg. use global instead of local when not needsSP
• Update NVPTX

LGTM!

(also, the langref says that the order within a priority is unspecfied. is something depending on this ordering (I assume so, since you bothered to change it)?

the commit description says "not ordered to group destructors with common associated objects" but it looks this patch is doing grouping by associated object. Is that intentional?
Also, maybe there should be a test which has destructors of different priorities associated with the same object?

After thinking a bit more, I'd like to rename DwarfExpression::addTargetIndexLocation to DwarfExpression::addWasmLocation because basically all of the methods on DarfExpression correspond to DW_OP_foo operands, so a method for adding DW_OP_wasm_location should probably just be called addWasmLocation. If we want to later have a utility function to extract some other functionality like converting a TargetIndex, we can always do that in the future when we have another caller.
With that suggestion, this change LGTM

I think that this approach to tracking the frame register through VReg and local substitution will work fairly reliably for now, because the frame register is live through the whole function and is unlikely to be coalesced with other locals or stackified.
One thing I don't like about D69807 is that getFrameRegister is called in a variety of places during generation of various bits of the code as well as in DwarfCompileUnit, whereas getFrameBaseLocation only has this particular meaning and used here (while in the other places getFrameRegister is returning phsyical registers that are meaningless partway through the pipeline.

• Split header generation into separate function

• Merge branch 'objcopy-wasm' into objcopy-add-remove
• Make Object own the OwnedContents instead of Section

• Simplify Section to a struct

• Remove OwnedContents for now, and give Writer::SectionHeader its correct name

Thanks for all your input!
Who else needs to review this? Can you say explicitly, so we can keep this from becoming one of those reviews that hangs in limbo for a long time without any clear path forward?

• review comments

• Review comments 3

I believe this patch is now also ready, please take a look.

• fix newline

• Split tests

I do find it odd that there is a PATH fallback in the existing code in the first place. I agree that basically no compiler other than the "system" compiler should ever use it (and also even the concept of the "system" compiler really only makes much sense on systems like Linux and BSDs where compiling things for the local system is common). I guess the other option here would be to just require that wasm-opt be in the same directory as clang, which we can arrange in wasi-sdk or wherever.

I believe I've addressed all the feedback now.

• Split Reader/Writer into separate files

• Review 2

fix dir name

• rename Wasm directories to wasm

Do you mean wasm instead of Wasm?

• Remove add/remove/dump section support

• Add test that removed section is gone
• Address round 1 of review comments

Thanks for the detailed review. I just uploaded a diff to address the comments (other than splitting the patch apart).

• Add test that removed section is gone

Ping. The target-independent portion of this patch is very small now.

Also if at some point we are able to remove a bunch of the driver logic from emcc and move it here, (e.g. running clang to link instead of lld directly) we'll need to watch out for this.

WRT the LTO directory name, there's theoretically the danger that someone (e.g. emscripten) could be doing a rolling release of the compiler and get invalidated within a major revision. But in that case, 1) they should be rebuilding their libraries on each release anyway, and 2) last time I checked, policy was to make auto-upgrade of bitcode work within a revision. So it's probably not a problem.

Ping. Any other opinions? Perhaps @dblaikie ?

LGTM on the approach, just one more question on the wasm-opt flags.

If the SDK is distributed with the compiler and version-locked, it seems like it should be ok.

@yurydelendik can you split and/or rename this patch?
@aprantl do you have any more feedback on the above, or any suggestions about who else might be interested?