Re: [PATCH v2 1/4] tools/tests/x86_emulator: avoid duplicate symbol error with clang

[Edwin Torok <edwin.torok@xxxxxxxxxx>]

> On 3 Mar 2026, at 13:59, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 27.02.2026 11:58, Edwin Török wrote:
>> --- a/tools/tests/x86_emulator/test_x86_emulator.c
>> +++ b/tools/tests/x86_emulator/test_x86_emulator.c
>> @@ -1882,8 +1882,13 @@ int main(int argc, char **argv)
>> #define decl_insn(which) extern const unsigned char which[], \
>>                          which##_end[] asm ( ".L" #which "_end" )
>> #define put_insn(which, insn) ".pushsection .test\n" \
>> -                              #which ": " insn "\n"  \
>> +                              ".ifndef "#which"\n" \
>> +                              #which ": \n" \
>> +                              ".endif\n" \
>> +                              insn "\n"  \
>> +                              ".ifndef .L"#which"_end\n" \
>>                               ".L" #which "_end:\n"  \
>> +                              ".endif\n" \
>>                               ".popsection"
> 
> Nice idea, but why multiple .ifndef, and why emitting the insn even if the
> labels are already there (and hence won't be emitted a 2nd time)?

I think we still need to execute the instructions, so they can be compared 
against the emulator.
(Especially that on the 2nd loop the instructions might get different inputs, 
 and thus potentially produce different outputs)

IIUC this will look like this:

// i=0 unrolled
asm volatile( // ... first instance defines the label and instructions, stores 
result)
x86_emulate(…)
… compare emulation and actual execution results

// i=1 unrolled
asm volatile (// … second instance, doesn’t define the label, just executes 
instructions, and stores result)
x86_emulate(…)
… compare emulation and actual execution results

If we don’t emit the instructions when the label is already present then the 
2nd time around we’ll have (stale) data from i=0.


> 
> Further, if the compiler unrolls a loop and instantiates such a put_insn()
> twice, it could pick different inputs (where flexibility is allowed). Most
> present uses (including ones I have pending) meet this requirement (i.e.
> permit only a single register per operand), but vmovdqu{32,16}_to_mem,
> evex_vcvtph2ps, vpcompress{d,q,w},  vpdpwssd_{vex1,vex2,evex}, and
> vmovsh_to_mem don't. {,v}movsd{,_masked}_to_mem, pcmp{e,i}str{i,m} and a
> few more could also end up being problematic if different addressing was
> used for the memory operand(s).
> 
> None of those sit in loops, I think, so we may be safe. But the constraints
> need properly writing down in a comment, I think.

Good point, there is an implicit assumption that multiple uses of the same 
“which” argument contain
exactly the same binary form.
If the forms are equivalent the test would still pass (but perhaps emulator 
test coverage will be reduced),
and worst case if they are different and not equivalent a test could fail.
If we start running these tests in a CI then we should notice if that happens 
with (future) patches.

Also if someone happens to typo the name of a ‘which’ in a new test in a way 
that it reuses an existing one then they’ll no
longer get a build failure (or 2 independent series, both adding tests for the 
same instruction).
Perhaps this one could be avoided if I extend put_insn (or decl_insn) to 
contain some C symbols that’d cause a conflict.

Will think about this and send a new version including that and the comment.

> 
>> @@ -5248,7 +5253,7 @@ int main(int argc, char **argv)
>>         memset(res + 3, ~0, 8);
>>         regs.eax = (unsigned long)res;
>>         regs.ecx = ~0;
>> -        for ( i = 0; i < 2; ++i )
>> +        for (i = 0; i < 2; ++i )
>>         {
>>             decl_insn(vmovsh_to_mem);
> 
> Excuse me?
> 

Sorry, should’ve noticed before sending (a stray change from undoing the 
volatile, wasn’t meant to be there).

Best regards,
—Edwin