• clarified the error message

• separated error message for v1 and unknown eflags
• added a test

I do not have a concrete proposal at the moment, but before fixing the error reporting interface, I want to make sure that it is as simple and less intrusive as possible. For example, a simple additional text processing to convert Unix-style error message to MSVC style might work (which is a bit hacky but the least intrusive). There are other ideas, perhaps. I'd like to explore more possibilities before setting my mind.

In any case, this change makes sense to me. Thanks!

The clang way: one movabsq plus a R_X86_64_RELATIVE relocation that is supposed to be resolved by the dynamic loader.
The GCC way: two instructions but no relocation to be resolved.

The clang way is still position-independent.

I noticed that MSVC link.exe doesn't show the error message in such a format that you can jump the source location when a link error occurs. Is this a configuration error in my environment, or is it expected?

Overall looking pretty good.

In D48095#1130635, @MaskRay wrote:

OK moved the test to x86-64-reloc-gotoff64.s, and renamed x86-64-reloc-got.s to `x86
gcc -O2 -S -mcmodel=medium -fPIC a.c emits the expected two instructions whose sum computes the pc-relative address (runtime address of _DYNAMIC)
clang emits a movabsq instruction which compiles to a R_X86_64_RELATIVE relocation after linking, which is not useful to the dynamic loader.

• simplify

• simplify

First of all, please do not remove x86-64-reloc-gotoff64.s. I'd write your new test to that file instead. It is easier to maintain two separate test files than having one larger test file.

Well, I still think separating the test is better to keep the test files manageable.

Could you create a new test file for the new test case?

• Remove O(n^2)-ness.

Test is not only important to not break the feature in the future, but it helps readers a lot by demonstrating the issue in action, so even for a change that's not ready for submission, adding test is greatly appreciated.

As always, can you add a test?

Let me try. I'll make a change and upload shortly.

LGTM

LGTM

Can you give me a pointer to the spec describing these relaxations?

It is not me who requested you remove GOTOFF64 relocation support from your patch, but if you have a patch with a title "Add R_X86_64_GOTPC{32,64} support", I wouldn't be surprised if you are requested to remove code to support other types of relocations, as patches shouldn't contain unrelated changes. A best practice is to separate a patch to two or more.

I applied this patch but the test failed. Can you fix?

Thank you for doing this. Though we probably should discuss the future plan at some point, this seems like a pure improvement to me.

LGTM

Could you add a test?

Well, I don't think that pointing out that we shouldn't convert strings between UTF8 and UTF16 back and forth is nitpicking, but yeah, this has already been addressed, so feel free to submit. LGTM.

Yes, I'll create a follow-up patch to remove RequiresNullTerminator from this function and create a new function for those that needs that functionality.

I believe this patch is ready for submit, though I didn't get any LGTM for this. Could you review it for me?

For now, bytecode offset (function or file relative) is used in role of the PC -- for call stack, for profiler, and we also successfully adapted it for source maps. I'm not sure what moving to index (of wasm operator?) base will change, but it will only will add complexity and will not solve this particular issue.

As to making it a separate tool to compress wasm text section, I believe that's an option, though I'm not sure if it is ideal. I imagine that the tool compresses text section by optimize LEB128-encoded numbers and then strip debug sections to produce production binary. That doesn't sound like a bad idea, at least, and maybe that's a good idea. But I'd like to hear from wasm developers about it, as it affects their workflow.

There's no notion of "PC" in wasm, no? You cannot take an address of a function and call it indirectly later. Well, you can do that, but what you get is not an address in the regular mean. It's an index, if I understand correctly. So, by PC, what do you mean?

I do imagine that "there's no way to create an executable with debug info if you are creating a release build" is unacceptable. So, we need to support both -O2 with debug info. But is this the only way to do this? This seems a bit too complicated to me. Maybe just code is complicated and the algorithm might not, but it is hard to tell because of the lack of any explanation of the algorithm.

LGTM

Is this OK?

So the best practice is, when you get a UTF-16 string from an Windows API, you should convert it to UTF-8 as soon as you can so that you can always process strings as UTF-8. Likewise, you should always keep all string in UTF-8 in memory and convert them to UTF-16 just before passing them to an Windows API if necessary. If you find yourself writing UTF-8 ↔ UTF-16 conversion code too frequently, that's like a signal that you are not doing text processing in UTF-8. Basically you need to do coding conversion up to only once for each new string.

It seems you converted the same string back and forth between UTF-8 and UTF-16 to call Windows functions and LLVM functions. That's not only a waste of time (which is not a big problem) but complicates the code.

Could you share a few real examples of error messages with and without this patch to see how they look like?

LGTM

What was the motivation for this new option? Given that it is easy to write run the same command again with /linkrepro, I don't see an immediate need to add this as a new option.

LGTM

LGTM

I would probably prefer this dumb but simple change over making a change to OptTable.cpp so that it synthesizes "Alias for ..." strings for alias options. If we have hundreds of lines of aliases, that might help, but for this 30+ options, maybe it's a bit too much?

LGTM

Do you have a plan to work on this, or did you just abandon your port?