Yeah, I don't have time to work on this now, and I don't have a good idea how to solve this either.

The code looks fine to me, and I trust that you have tested it.

Sounds like a good diagnostic to me. For the warning message perhaps it can be changed a bit to avoid using a full stop?

Update commit message and comment to reflect latest findings from D149549.

In D149549#4339015, @mstorsjo wrote:

@alvinhochun managed to pinpoint and guess the differing issue.

With WinSDK/UCRT 10.0.19041.0, linking fails, since references to __stdio_common_vfprintf pulls in output.obj from libucrt.lib, and this object file pulls in wcstol from strtox.obj, which then later triggers the duplicate definition.

With libucrt.lib from WinSDK 10.0.22621.0, this no longer happens. But the test still failed, fulfilling the XFAIL, with the following error:

AddressSanitizer: CHECK failed: asan_rtl.cpp:387 "((!asan_init_is_running && "ASan init calls itself!")) != (0)" (0x0, 0x0) (tid=16328)
    <empty stack>

After this patch, this no longer triggers errors, and the test runs successfully. So the original XFAIL condition is dependent on WinSDK version, but there was a second error which made the XFAIL still seem to behave correctly, until after this change.

I can't comment on the rest of whether it's expected that this error condition that previously aborted execution, now no longer triggers a failure.

Thanks for adding me in the loop. The change itself looks fine, though I haven't tried it on MinGW-w64 target yet. Some comments on the tests:

In D150349#4335307, @vitalybuka wrote:

typo: profided

but on Windows the static asan runtime does intercept static copies of libc functions

can this change in future?

Weirdly, that test XFAILs here on my i686-windows-msvc environment with check-asan (static runtime) because of different reasons:

In D149998#4331540, @mstorsjo wrote:

I guess target might be anything, for cases that are configured to cross compile by default, so we shouldn't rely on that in this test. I wonder if there's some similar feature we could check for the host instead of the target? Or perhaps we should just mark the windows part of the test UNSUPPORTED: target={{.*-windows-gnu}} and not complicate things more than necessary - that should be enough to achieve what we want here...

Since there have been no other comments, I'll go ahead and mark this accepted.

Rebase and apply suggestions.

Apply suggestions.

Apply suggestions: remove TODO, put negative tests in the same file as positive tests.

Looks like a straightforward test fix.

In D149579#4320765, @MaskRay wrote:

OK. I think after D149695 (landed), D149920, and this patch D149579, these cases should be correct.

Rebase and add test cases with offset operator, and some TODOs/FIXMEs for comment

LGTM except for the issue with the test. I guess wait a few days to see if anyone else have other comments, or someone else can accept too.

Thanks, LGTM.

In D149579#4313475, @MaskRay wrote:

It looks we accept a case (t2, w/o or with the patch) that GNU as rejects.

% as -msyntax=intel =(printf 'call qword ptr [rip + fn_ref]') -o gas.o
/tmp/zshjfuv2e: Assembler messages:
/tmp/zshjfuv2e: Warning: end of file not at end of a line; newline inserted
/tmp/zshjfuv2e:1: Error: invalid operands (*UND* and *UND* sections) for `+'

It does work if you add .intel_syntax noprefix to the assembly or pass -mnaked-reg to as.

Yes, what jhenderson said. Also you can see in the pre-merge checks it fails to apply the patch, because what it does is that it first applies the parent patch before applying this patch.

In D149610#4312974, @aganea wrote:

@hans @alvinhochun The rationale is to simply remove noise from the output when there are sparse ordinals in the table. I'm not quite sure how useful is to keeps those empty entries, since they will be skipped over by the kernel anyway. [...]

In D149610#4311847, @alvinhochun wrote:

I don't think we need to follow exactly the behaviour of dumpbin. Can you check what Binutils objdump does just for comparison?

objdump skips over empty values, as this patch does (using a test added by D149611):

Turns out this change affects MS inline assembly and possibly other inline assemblies as well. Looking at ms-inline-asm-functions.c (on Compiler Explorer - https://godbolt.org/z/3xfW75Go4):

Apply changes from parent patch

Address some comments

I don't think we should skip empty exports, especially not by checking only the RVA, because it looks like it is theoretically possible to have RVA = 0 but a non-empty name for an export. Even entries with null RVA and null name seems worth keeping as they do take up space in the export table.

I'm aware of the check-clang failure in CodeGen/ms-inline-asm-functions.c. It seems to be caused by the MS inline assembly being translated in a way that breaks with this patch, which means I may have to look into fixing that code too. That also revealed an issue with my patch regarding how call [offset function] is handled -- apparently GAS treats it the same as call function. I'll try to get this fixed.

Format the commit message better

Ping, can someone accept this revision?

Run clang-format

Split the bit making unknown instructions non-fatal into D149549, and change the way errors are reported to avoid direct dependency from interception to sanitizer_common. Now asan registers a callback in interceptor to call __sanitizer::Report.

LGTM

Superseded by D149064.