r198858 - Disable LeakSanitizer in TableGen binaries, see PR18325

[Alp Toker]

On 09/01/2014 19:49, Chandler Carruth wrote:
> On Thu, Jan 9, 2014 at 10:07 AM, Alp Toker <alp at nuanti.com 
> <mailto:alp at nuanti.com>> wrote:
>
>
>     On 09/01/2014 17:52, Jordan Rose wrote:
>>
>>     On Jan 9, 2014, at 9:42 , Alp Toker <alp at nuanti.com
>>     <mailto:alp at nuanti.com>> wrote:
>>
>>>
>>>     On 09/01/2014 17:30, Jordan Rose wrote:
>>>>
>>>>     Would it help if the function were pre-declared in a system
>>>>     header, and then just implemented or not implemented in user code?
>>>
>>>     Hi Jordan,
>>>
>>>     That's the current situation -- as long as sanitizer headers are
>>>     available and in use the part of the spec you highlighted means
>>>     it's acceptable.
>>>
>>>     The problem arises when sanitizer headers aren't available.
>>
>>     I just don't think the program is illegal when LEAK_SANITIZER is
>>     not defined. The tokens within the #ifdef are skipped completely,
>>     so they don't refer to names and certainly don't declare anything.
>
>
>     If there was an ifdef in TableGen there'd be no problem. The
>     trouble is there is no ifdef...
>
>
> Ok, this thread has grown quite a bit, but let me try to summarize and 
> see if I've missed anything....
>
> I see two specific concerns here:
>
> 1) An immediate problem that you have some internal build environment 
> which this use of a reserved name breaks.
>
> 2) You have a higher-level problem with the design of the interface 
> using reserved names.
>
>
> Let's try to address these separately, because they seem to call for 
> different immediacy of response.
>
> To the first point, while I don't think that this merits a revert (we 
> typically want a build bot or something to do that), it certainly 
> seems reasonable to support. I was trying to discuss the best way of 
> supporting it: if Kostya were comfortable with LSan providing a macro, 
> that would be one way, but he isn't and I'm perfectly happy adding a 
> CMake and/or configure&make mechanism to control this with a 
> preprocessor macro. If this seems like the right approach, let me know 
> and I'll do that.

I'm fine to re-land the commits with the ifdef if that's the way you 
want to go. Seems to be missing the point somewhat but..

The build environment that threw up the issue here isn't yet pushed to 
clang upstream (hopefully soon as next week!) but it's caught a 
legitimate bug and the code in those commits does violate the standard.

>
>
> To the second point, I think we have a more fundamental disagreement 
> about the priority of different concerns in the design constraints. My 
> priority is to ensure the API is appropriately insulated from 
> potential user code that might want to use a particular name in its 
> code. The mechanism used for this is the one used in many places: 
> putting the names into the space of reserved names. I understand that 
> you would design the interface the other way with the other set of 
> tradeoffs, but there doesn't seem to be either:
>
> a) A technically superior way to address the legitimate concern of the 
> LSan interface names colliding with user code's names, or
> b) A strong consensus across the community that this is the wrong 
> design priority.

I'm fine to give guidance on this if you want my help. It certainly 
seems like this part of the sanitizer interface is intended to be 
consumed/defined as a C library, so ordinary C API naming rules should 
do the trick with regards to a conflict.

That said I don't use the feature so whatever you get up to there is up 
to you as long as it doesn't introduce problems to the main build :-)

Is there anything I missed here?

Alp.


>
> Given that, I think we may have to disagree and move forward. Notably, 
> as Kostya is the primary maintainer for the sanitizers, I think that 
> in this kind of a disagreement it makes sense to simply defer to his 
> judgement.
>
> -Chandler

-- 
http://www.nuanti.com
the browser experts