Address feedback

Thanks!

Change diagnostics for conflicting swift_name, add a test case

Address everything but warning

Address feedback from @aaron.ballman

I think that tracking the segment length should be okay. It’s a total overhead of 8 bytes, which is only allocated once, however, the naming here is egregiously bad. This is the segment length and not the DSO length nor the DSO mapped length (which may actually be larger or smaller based on the load segment layout).

Address feedback

• make the attribute inheritable
• add a test case for inheritance
• add a test case for C++ type alias

• Add additional test case that was requested

Address some feedback from @aaron.ballman

Add additional test case

The make based build was long ago deprecated.

Address feedback

LGTM.

address feedback from @aaron.ballman and @gribozavr2

Discussed this with @aaron.ballman offline; he is okay with this since this is not going to permit incorrect code through without a diagnostic, even if it is not the most clear and the attribute wont be accidentally ignored. We can add more diagnostics as a follow up if it is useful.

Address @gribozavr2's comments

Address feedback from @aaron.ballman

Yes, sorry I wasn't clear about that, but you understood correctly. I was suggesting using -1 as the search all, since it doesn't make sense as a DSO base. Minor comment about a possible constexpr improvement, but doesn't require a re-review.

I was thinking more the SjLj backend. I am okay with an additional option, but this currently would force enable the SEH backend.

Is there a funnel point that we know that all the rebase entries will pass through to ensure that we catch them rather than adding them at a couple of sites?

It is definitely possible to disable SEH via /EHs- though

Why not use -1 as the indicator instead of 0?

This seems relatively harmless, not sure if its valuable though.

• correct typos
• apply clang-format changes
• const correct a few declarations.

The change itself is fine, but what about downstream projects which define the option? Will that trigger a warning?

LGTM with the name change.

I don't think that this is entirely true. I believe it is possible to disable SEH with cl, so I think that it is better to conditionalize it as if defined(__SEH__) || defined(_SEH) since IIRC, _SEH is the define that cl uses.

Sounds good to me; LGTM with the rephrased commit message.

Please ensure that the fix goes into 11 before release (CC: @hans), but the remainder of the change is fine.

Can you please verify that findUnwindSectionsByPhdr was not exposed previously? This seems like it could be an ABI break.

I think that the confusing part is that it is not that they are C functions, they are still assembly, but with C calling convention. I think that you should state that in the description as it currently reads as though they are written in C, which they are not.

The idea itself is reasonable. The summary description is very confusing would you mind reworking that? Could you check what happens for the jumpto member? Ideally it would just be an alias for the function to avoid a frame setup and unnecessary jump. I think that is the only thing that really stands out of needing work in the implementation.

LGTM with the renaming of the local variable. Thanks for implementing this (it was the next item I wanted to add)!

Good idea

This looks fantastic! I'm really liking the better diagnostics. The only thing I'd change is the "symbol kind %0 unsupported on COFF targets". I think that "relocation variant" is more precise than "symbol kind". Thanks for improving this.

Given that the original motivation here was to help with NetBSD, and the submodule visibility flag handling unblocks NetBSD, I think that I will hold off on this unless some other motivation arises.

Address review feedback