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 16:43, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 03.03.2026 16:55, Edwin Torok wrote:
>>> On 3 Mar 2026, at 15:36, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>> On 03.03.2026 16:09, Edwin Torok wrote:
>>>>> 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.
>>> 
>>> Of course, but they cannot be executed without having a label. We use the
>>> label to point the emulated IP there, and then we use the end label to
>>> check that after emulation the emulated IP has advanced as expected.
>> 
>> Oh that means that we won’t actually be testing anything useful in 
>> iterations>=1
>> (the test passes, but it runs the same test as it did on iteration 0).
> 
> May I ask for a little less bold statements?


Fair enough, I probably still don’t fully understand how this works, and I tend 
to jump to conclusions too soon.

> Of course the 2nd iteration isn't
> identical to the 1st. The insn encoding is the same, but the operands (the 
> mask
> in particular, i.e. the value %k3 holds) aren't.
> 


If we’re very careful with how we use put_insn() then it might work (e.g. with 
more comments as you suggested). 

I think it could go wrong if the compiler would emit a constant that is 
different in each loop iteration:
the emulator would be given the same binary sequence to emulate, because it 
always refers to the label from the 1st iteration
(the only label that exists with my patch), and therefore the value is the 
constant is the one from the first iteration.
Whereas the actual execution would use the actual values of those constants, 
which are different in each loop iteration.
This may result in a detectable change in the result between the emulator and 
actual executions.
If instead of a constant, it emits a reference to a register then the problem 
wouldn’t exist, because the register would contain the correct value on each 
iteration.

If we’re careful to use the appropriate constraints then the problem can be 
avoided (e.g. I assume a register constraint means that the compiler must 
always put the value into a register first and use that, even when the value 
would be a compile-time constant). But the macro itself can’t control what 
constraints are used.

Although it might work, this approach is quite brittle. For now I sent a V3 
which uses -O0 for test_x86_emulator.o, which is much simpler.
-Os would work too, if I put it into the correct place (has to go into 
HOSTCFLAGS, not CFLAGS), but then we rely on the optimiser to always correctly 
estimate the size and avoid duplicating the assembly block. Since this isn’t 
performance critical I suggest using -O0.

If a better solution is found then this can be revisited again (e.g. one based 
on .ifndef as discussed here)


Best regards,
—Edwin