[PATCH] D34993: Hack to keep __real_foo

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/ELF/wrap.s:23
+// SYM:      Name: __real_foo
+// SYM-NEXT: Value: 0x11020
 // SYM:      Name: __wrap_foo
----------------
jhenderson wrote:
> ruiu wrote:
> > jhenderson wrote:
> > > This doesn't look right to me at all. `__real_foo`, `__wrap_foo` and `foo` can't all have different values. Surely there should be only two different values between these three, because there are only two functions in the code?
> > `__wrap_foo` and `foo` should have the same value, but `__real_foo` shouldn't, because it should have a value of the original function instead of your wrapper function. So I think this is correct.
> I disagree with this. The current meaning of --wrap in other linkers is to redirect references to symbols, not to change the symbols themselves, so if anything, `__real_foo` and `foo` should have the same value and `__wrap_foo` a second, different value. (As an aside, the current test has three different values, not two, so is definitely wrong).
> 
> I don't understand why we need __real_foo at all - it's not actually a function in the user's source for example.
> 
> There is a serious danger that this could confuse users when they try to debug their code. By having multiple symbols for the same address, there is a potentially negative impact on the call stack displayed by the debugger - it has the right to pick whichever symbol it feels like (it may not even be deterministic about this). For example, this could mean that when the code crashes in the __wrap_foo() function, it could say that the crash occurs in foo(). The user then stares at the foo() function in their source code and cannot see anything wrong with it, because the problem isn't there.
Rafael posted the following on the mailing list, so I've included it here for completeness:
> This actually implements the same behavior as bfd. The symbol references
> become:
>
> ```
> 201000:       ba 10 10 01 00          mov    $0x11010,%edx
>  201005:       ba 10 10 01 00          mov    $0x11010,%edx
>  20100a:       ba 00 10 01 00          mov    $0x11000,%edx
> ```
>
> And the symbols in the symbol table are unchanged:
>
> ```
> 0000000000011000 A foo
> 0000000000011020 A __real_foo
> 0000000000011010 A __wrap_foo
> ```
>
> Cheers,
> Rafael

Oh, I see what's going on now! I'd completely missed the second input to the test that defined all three symbols, so I was incorrectly assuming that only `__wrap_foo` and `foo` were defined. My apologies for the confusion.

FWIW, if `__real_foo` isn't defined in the test input, then it's not included in the bfd output, which was the case I was thinking of. It feels like this test case is missing.

I still disagree with @ruiu about `__wrap_foo` and `foo` having the same value //in the symbol table// (I don't think they should, and neither does bfd), although of course references to them in the code should be patched to the same value (i.e. to `__wrap_foo`) when using --wrap.

I now have no objection to this test change.


https://reviews.llvm.org/D34993