[Bug sanitizer/56454] New: need to rename attribute no_address_safety_analysis to no_sanitize_address

[Bug sanitizer/56454] New: need to rename attribute no_address_safety_analysis to no_sanitize_address

[kcc at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56454

             Bug #: 56454
           Summary: need to rename attribute no_address_safety_analysis to
                    no_sanitize_address
    Classification: Unclassified
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: sanitizer
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: kcc@gcc.gnu.org
                CC: dodji@gcc.gnu.org, dvyukov@gcc.gnu.org,
                    jakub@gcc.gnu.org, kcc@gcc.gnu.org


Clang r176076 has renamed the disable-address-sanitizer attribute from 
no_address_safety_analysis to no_sanitize_address 
(the old syntax is still working, but is declared deprecated). 
http://clang.llvm.org/docs/AddressSanitizer.html#attribute-no-sanitize-address

gcc needs to match the clang syntax for better interoperability. 
Hopefully, we can make this change before 4.8

[Bug sanitizer/56454] need to rename attribute no_address_safety_analysis to no_sanitize_address

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56454

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> 2013-02-26 07:14:42 UTC ---
What is the reasoning for the change?  I think it is bad form to change the
attribute name at all.

[Bug sanitizer/56454] need to rename attribute no_address_safety_analysis to no_sanitize_address

[kcc at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56454

--- Comment #2 from Kostya Serebryany <kcc at gcc dot gnu.org> 2013-02-26 07:19:00 UTC ---
Long story short: 
we needed to unify the names for asan/tsan/msan attributes. 
Now they are called
no_sanitize_address
no_sanitize_thread
no_sanitize_memory

Lengthy discussion: 

http://clang-developers.42468.n3.nabble.com/clang-attributes-to-disable-asan-tsan-msan-td4030472.html

[Bug sanitizer/56454] need to rename attribute no_address_safety_analysis to no_sanitize_address

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56454

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> 2013-02-26 07:25:40 UTC ---
(In reply to comment #2)
> Long story short: 
> we needed to unify the names for asan/tsan/msan attributes. 
> Now they are called
> no_sanitize_address
> no_sanitize_thread
> no_sanitize_memory

I think it was change to change the name of them considering the more verbose
names seem better than no_sanitize_address .

[Bug sanitizer/56454] need to rename attribute no_address_safety_analysis to no_sanitize_address

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56454

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> 2013-02-26 07:26:44 UTC ---
Oh and this really should have been talked about on the GCC list too.  Please
next time bring these issues up at the same time on the GCC list for more
feedback instead of considering just clang's thoughts.

[Bug sanitizer/56454] need to rename attribute no_address_safety_analysis to no_sanitize_address

[kcc at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56454

--- Comment #5 from Kostya Serebryany <kcc at gcc dot gnu.org> 2013-02-26 07:38:42 UTC ---
Hm... Ok... 
Although there is a risk that this way we may never reach a decision.
Is there a precedent of solving issues this way? 

How about __has_feature, by the way? 
Should we start this discussion on the two lists? 
(reminder: clang uses #if __has_feature(address_sanitizer), gcc uses #if
__SANITIZE_ADDRESS__)

Clang long ago refused to use a cpp macro and insisted on __has_feature.
My several feeble attempts to ask for __has_feature in gcc were mostly ignored.
I don't care which one is used, but the current thing (below) is ugly. 

// GCC does not understand __has_feature.
#if !defined(__has_feature)
# define __has_feature(x) 0
#endif

// Clang understands __has_feature, GCC defines __SANITIZE_ADDRESS__
#if __has_feature(address_sanitizer) || defined(__SANITIZE_ADDRESS__)

[Bug sanitizer/56454] need to rename attribute no_address_safety_analysis to no_sanitize_address

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56454

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> 2013-02-26 07:57:13 UTC ---
(In reply to comment #5)
> Hm... Ok... 
> Although there is a risk that this way we may never reach a decision.
> Is there a precedent of solving issues this way? 
> 
> How about __has_feature, by the way? 

_has_feature is a useless feature considering preprocessed macros exists. Maybe
the clang folks want an unified way of finding if a feature exists but that is
up to clang.  We don't.  Also the attribute renaming is a different story and
really should never happen once it is out in the wild which it is now.  That is
the point I am trying to make once you have it out there it should stay even if
it is a bad name.  We have bad names for options and only change them in the
rare case (-Wconversion was the only rare case that comes to mind).

[Bug sanitizer/56454] need to rename attribute no_address_safety_analysis to no_sanitize_address

[kcc at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56454

--- Comment #7 from Kostya Serebryany <kcc at gcc dot gnu.org> 2013-02-26 08:07:16 UTC ---
Your comment #6 explains why I did not CC gcc lists to the clang discussion. :)
There are too many strong opinions on both sides and I don't want to 
be an instigator of a holy war. 
I'd rather have incompatible syntax than spend weeks in non-productive emails.

Also, we are not removing the old name, we are just documenting it as
deprecated.
Which means that it will stay there for some number of releases.

[Bug sanitizer/56454] need to rename attribute no_address_safety_analysis to no_sanitize_address

[manu at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56454

Manuel López-Ibáñez <manu at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |manu at gcc dot gnu.org

--- Comment #8 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2013-02-26 08:40:51 UTC ---
(In reply to comment #7)
> Also, we are not removing the old name, we are just documenting it as
> deprecated.
> Which means that it will stay there for some number of releases.

Why not accept it silently and leave them there forever? And create a new alias
in GCC without removing the current one?

[Bug sanitizer/56454] need to rename attribute no_address_safety_analysis to no_sanitize_address

[kcc at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56454

--- Comment #9 from Kostya Serebryany <kcc at gcc dot gnu.org> 2013-02-26 08:46:36 UTC ---
Clang accepts the old name silently. 
I am not sure about "forever", 
clang will probably start printing a "deprecated" warning at some point.

[Bug sanitizer/56454] need to rename attribute no_address_safety_analysis to no_sanitize_address

[manu at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56454

--- Comment #10 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2013-02-26 11:19:34 UTC ---
(In reply to comment #9)
> Clang accepts the old name silently. 
> I am not sure about "forever", 
> clang will probably start printing a "deprecated" warning at some point.

Well, I would say get what you can, and fight the fights that you can win. So
if you can get the alias, go for that first. Only after that, try to get the
old name deprecated, if possible. GCC maintainers do not really care what clang
does, so framing arguments in that way will get you negative feed-back. The
only important arguments for GCC maintainers are: who is going to implement it
and what is the benefit for gcc.

That clang gives a deprecated warning is not an issue for gcc. That clang uses
a different attribute is an issue of compatibility, so there is maybe an
argument for adding an alias to gcc.

(And for the same reasons, I agree that raising the issue in both Clang and GCC
lists would have been a waste of time.)

[Bug sanitizer/56454] need to rename attribute no_address_safety_analysis to no_sanitize_address

[kcc at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56454

--- Comment #11 from Kostya Serebryany <kcc at gcc dot gnu.org> 2013-02-28 12:01:34 UTC ---
Created attachment 29550
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29550
rename no_address_safety_analysis to no_sanitize_address

How do we decide? When is the deadline for 4.8? 
I've attached a patch that implements the new attribute and renames it
in the tests and in the docs, but still understands the old name

(I will not be able to submit the patch until March 11)

[Bug sanitizer/56454] need to rename attribute no_address_safety_analysis to no_sanitize_address

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56454

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-02-28 12:13:42 UTC ---
(In reply to comment #11)
> Created attachment 29550 [details]
> rename no_address_safety_analysis to no_sanitize_address
> 
> How do we decide? When is the deadline for 4.8? 
> I've attached a patch that implements the new attribute and renames it
> in the tests and in the docs, but still understands the old name
> 
> (I will not be able to submit the patch until March 11)

This doesn't DTRT with the old attribute, because you lookup_attribute only the
new name.
Either you'd need to look up also the old name, or
handle_no_address_safety_attribute would need to add the new name of the
attribute manually (and perhaps don't add the old name).

I can take care of it (and perhaps also introduce no_sanitize_thread attribute
- what exactly should be done when it is set?  Instrument atomics always, and
not instrument anything else?).

OT, are you ok with incrementing the kMidMemEnd value (see my mail from Friday
last week)?

[Bug sanitizer/56454] need to rename attribute no_address_safety_analysis to no_sanitize_address

[kcc at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56454

--- Comment #13 from Kostya Serebryany <kcc at gcc dot gnu.org> 2013-02-28 12:36:12 UTC ---
> This doesn't DTRT with the old attribute, because you lookup_attribute only the
> new name.
> Either you'd need to look up also the old name, or
> handle_no_address_safety_attribute would need to add the new name of the
> attribute manually (and perhaps don't add the old name).
> 
> I can take care of it
That would be awesome!

>  (and perhaps also introduce no_sanitize_thread attribute
> - what exactly should be done when it is set?  Instrument atomics always, and
> not instrument anything else?).

no_sanitize_thread is actually not implemented in clang either yet
(i.e. clang understands this attribute, but the instrumentation ignores it)

The plan is to not instrument plain loads/stores when the attribute is set,
while still instrumenting atomics and function entry/exit. 

> 
> OT, are you ok with incrementing the kMidMemEnd value (see my mail from Friday
> last week)?

Sorry, I missed the message. replied there.

[Bug sanitizer/56454] need to rename attribute no_address_safety_analysis to no_sanitize_address

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56454

--- Comment #14 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-02-28 21:23:30 UTC ---
Author: jakub
Date: Thu Feb 28 21:23:23 2013
New Revision: 196361

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=196361
Log:
    PR sanitizer/56454
    * asan.c (gate_asan): Lookup no_sanitize_address instead of
    no_address_safety_analysis attribute.
    * doc/extend.texi (no_address_safety_attribute): Rename to
    no_sanitize_address attribute, mention no_address_safety_analysis
    attribute as deprecated alias.

    * c-common.c (handle_no_sanitize_address_attribute): New function.
    (c_common_attribute_table): Add no_sanitize_address attribute.
    (handle_no_address_safety_analysis_attribute): Add
    no_sanitize_address attribute, not no_address_safety_analysis
    attribute.

    * g++.dg/asan/default-options-1.C (__asan_default_options): Use
    no_sanitize_address attribute rather than no_address_safety_analysis.
    * g++.dg/asan/sanitizer_test_utils.h
    (ATTRIBUTE_NO_ADDRESS_SAFETY_ANALYSIS): Likewise.
    * c-c++-common/asan/attrib-1.c: Test no_sanitize_address attribute
    in addition to no_address_safety_analysis.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/asan.c
    trunk/gcc/c-family/ChangeLog
    trunk/gcc/c-family/c-common.c
    trunk/gcc/doc/extend.texi
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/c-c++-common/asan/attrib-1.c
    trunk/gcc/testsuite/g++.dg/asan/default-options-1.C
    trunk/gcc/testsuite/g++.dg/asan/sanitizer_test_utils.h

[Bug sanitizer/56454] need to rename attribute no_address_safety_analysis to no_sanitize_address

[y.gribov at samsung dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56454

Yury Gribov <y.gribov at samsung dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |y.gribov at samsung dot com

--- Comment #15 from Yury Gribov <y.gribov at samsung dot com> ---
Kostya, can you close?

[Bug sanitizer/56454] need to rename attribute no_address_safety_analysis to no_sanitize_address

[kcc at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56454

Kostya Serebryany <kcc at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED

--- Comment #16 from Kostya Serebryany <kcc at gcc dot gnu.org> ---
(In reply to Yury Gribov from comment #15)
> Kostya, can you close?

Indeed.