On 09/01/2014 16:49, Kostya Serebryany wrote:
On Thu, Jan 9, 2014 at 8:03 PM, Alp Toker <[email protected]
<mailto:[email protected]>> wrote:
On 09/01/2014 15:23, Kostya Serebryany wrote:
On Thu, Jan 9, 2014 at 6:57 PM, Alp Toker <[email protected]
<mailto:[email protected]> <mailto:[email protected]
<mailto:[email protected]>>> wrote:
On 09/01/2014 12:13, Chandler Carruth wrote:
Who controls / defines the interface?
The sanitizers.
Can you report a bug with them?
That's us (no need to file a separate bug unless you want to)
That's fantastic. A proper resolution might be in sight in that
case..
Moreover, it's blocking work on a practical level by
triggering
legitimate build issues in at least one configuration.
Removing
the function doesn't break anything and resolves the issues. I
don't see the problem here?
Will you get unblocked if we put this code under #ifdef
LEAK_SANITIZER ?
Alternatively you may probably build the code with
-D__lsan_is_turned_off=something_else
If conditionalizing turns out to be necessary, it seems best to
not to enable the function definition unless it's explicitly leak
sanitizer build.
However I'm starting to understand what you were trying to achieve
and have a much better proposal below..
Do you have suggestions how to improve the interface
We should be able to find a name or mechanisms to do this
properly, given that you have control over the interface.
w/o risking to clash with user-defined names?
That right there appears to be the point of confusion.
The problem is that you've required users to declare a function
name that should have been reserved for the implementation.
I said earlier this appears to be a "thinko" in the sanitizer
interface. I said that because, for the majority of sanitizer
functions, you're dealing with the reverse problem, and that
appears to have leaked into the naming of these functions
unintentionally.
So it's not a question of avoiding a clash with user-defined
names, but the other way round.
In summary, the problem you need to solve is identical to the
naming choices when declaring an entry point in a public C
programming interface.
Try naming this like any C API function (which is what it is) and
you'll have a solution. Some good LLVM-style function name
conventions are "include/llvm-c/Core.h" assuming you want to go
with the LLVM CamelCase style.
With this approach, you have the same risk of conflict as any C
function, which is a well-understood problem space. The sanitizer
is then free to give this symbol name special handling internally
without imposing on others.
If you do this there's no need to conditionalize, but please
retain the comment explaining the purpose so it doesn't get
"cleaned up".
Thanks for inviting a discussion Kostya. Does that sound good to you?
So, you propose to name this function lsan_is_turned_off (or some
such) instead of __lsan_is_turned_off?
include/sanitizer/lsan_interface.h- // The user may optionally provide
this function to disallow leak checking
include/sanitizer/lsan_interface.h- // for the program it is linked
into (if the return value is non-zero). This
include/sanitizer/lsan_interface.h- // function must be defined as
returning a constant value; any behavior beyond
include/sanitizer/lsan_interface.h- // that is unsupported.
include/sanitizer/lsan_interface.h: int __lsan_is_turned_off();
The flaw is above. The user can't portably provide this function because
declaring or defining a function with the reserved name is undefined
behaviour.
That function is very different from the rest of lsan_interface.h and
the correct way to provide it is as a C function with an ordinary
API-style name that the user can legally define.
That will work, but then there is a non-zero chance that a user will
already have such function for whatever purposes of his own.
There is always a non-zero chance of conflict with any C function.
LLVMContextCreate() has been part of the LLVM C API for years and
conflict hasn't been an issue, any more than it would be with a
LSANShouldDisableSanitizer() weak function.
To put this another way, even if "__" were hypothetically not reserved,
your approach would still be flawed because the user might as well have
declared a __lsan_is_turned_off() function for whatever reason.
In this case, the problem is bigger because "__" is reserved, so what
you have now has a 100% chance of conflicting by definition.
My suggestion is that, if it's important to you to not to introduce
unprefixed names in lsan_interface.h, just put the
LSANShouldDisableSanitizer() declaration in a separate lsan_control.h
header file that's include only at the point of use in a single TU.
What's worth, he will not know about the clash at the link time -- his
program will start behaving wrongly.
The __ prefix is exactly the tool that allows us to avoid a clash with
user-named objects.
The "__" names are reserved for the implementation.
Right now there's a function declaration sitting in the middle of
TableGen defining a function with a reserved name which is orthogonal to
what you want.
Alp.
Or I've got you wrong?
Alp.
--kcc
Anyway, I'm returning to the office to deal with this and I'm
disappointed you've tried to push it through unilaterally with
disregard for anyone outside of your own circle.
Alp.
-- http://www.nuanti.com
the browser experts
--
http://www.nuanti.com
the browser experts
--
http://www.nuanti.com
the browser experts
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
