[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
----------------
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.


https://reviews.llvm.org/D34993