Implement no_sanitize_address for global vars

[dougk]

Summary

This was already submitted as r284272.

Regarding the use of Attr as a local variable name, I would prefer to remain consistent with the existing code in CodeGenFunction.cpp which was not touched by this patch.

line 720: for (auto Attr : D->specific_attrs<NoSanitizeAttr>())

but I'm certainly not opposed to changing both places.

[dougk]

Also note: In this change, the diagnostic messages are correct; it's the enum name that's bad. The diagnostic message is wrong for the 'section' attribute, which is fixed by https://reviews.llvm.org/D26459

[aaron.ballman]

lib/CodeGen/SanitizerMetadata.cpp

68

Should use const auto * (feel free to change the other use(s) in a separate commit, no review required).

lib/Sema/SemaDeclAttr.cpp

5316

This formatting change is unrelated.

5343

You diagnose this as an error, but don't early return if the attribute is invalid. Is that intentional?

5364

You diagnose it as an error, but then add the attribute anyway. Is that intentional?

test/CodeGen/asan-globals.cpp

10

You modified no_sanitize_address, but did not add any test cases for it.

test/SemaCXX/attr-no-sanitize-address.cpp

24

Please add a new test case to replace this one, showing that the attribute is properly diagnosed when applied to something the attribute cannot appertain to.

test/SemaCXX/attr-no-sanitize.cpp

5 ↗

(On Diff #77357)

Please add a new test case to replace this one, showing that the attribute is properly diagnosed when applied to something the attribute cannot appertain to.

[dougk]

changes per Aaron Ballman

[kcc]

Does this change deserve a documentation update?

[dougk]

add a sentence about the change in AddressSanitizer.rst

[aaron.ballman]

lib/Sema/SemaDeclAttr.cpp

5348

This is unfortunate because the declarative subject information from Attr.td does not match the semantics in SemaDeclAttr.cpp for many of the sanitizer names. For instance, this means that someone using __attribute__((no_sanitize("thread"))` on a static local variable will get a diagnostic telling them that it only applies to functions, methods, and global variables. If they make their static local variable into a global variable, they will then get a different diagnostic telling them, oops, just functions or methods.

While I understand that this was already submitted in r284272, I don't see any rationale given in the commit log as to why this behavior is desirable in the first place. Can you give me some background information on why this is needed? Does it apply to other sanitizer names as well?

[dougk]

Suppression of sanitizing is necessary if the variable is magically a memory-mapped device I/O address.
The linker can arrange for this to be the case using fancy scripts, or even just as simple as a section attribute that requires that you take up exactly a certain number of bytes in the section.
There was some thought that any non-default section should preclude sanitization, but Kostya said that, no, it would make sense to require explicit no-sanitize. I (mistakenly) took that to mean "just do it", for which I apologize.

lib/Sema/SemaDeclAttr.cpp

5316

reverted. (clang-format-diff did that on account of proximity to the added lines)

5343

not intentional. fixed

5364

not intentional. fixed

test/SemaCXX/attr-no-sanitize-address.cpp

24

that's already tested by noanal_testfn which has no_sanitize_address on 'int x = y';

[aaron.ballman]

In D26454#609578, @dougk wrote:

Suppression of sanitizing is necessary if the variable is magically a memory-mapped device I/O address.
The linker can arrange for this to be the case using fancy scripts, or even just as simple as a section attribute that requires that you take up exactly a certain number of bytes in the section.
There was some thought that any non-default section should preclude sanitization, but Kostya said that, no, it would make sense to require explicit no-sanitize. I (mistakenly) took that to mean "just do it", for which I apologize.

Thank you for the explanation, but I'm still wondering if this applies to only address sanitization, or to others as well, such as memory or thread. The fact that the declarative information in Attr.td is incorrect now is a concern. We can either address that concern by making the conflict go away for all sanitizer strings (which may not be sensible), or by making some complex changes to the way subject lists work (which may be more work than it's worth). The point to having a single no_sanitize attribute was so that we didn't need to add a new attribute every time we came up with a new sanitizer, but it was understood that this works because all of the sanitizers apply to the same constructs. This change breaks that assumption so that now one of the sanitizer strings diverges from all the rest, and I would like to avoid that.

[dougk]

I think it probably works to have the attribute appertain to any sanitizer. I did not know that it did, so I conservatively assumed that it didn't. I'll go ahead and make things consistent, and confirm that it dtrt.

lib/CodeGen/SanitizerMetadata.cpp

   D.printQualifiedName(OS);
 
   bool IsBlacklisted = false;
-  for (auto Attr : D.specific_attrs<NoSanitizeAttr>())
+  for (const auto *Attr : D.specific_attrs<NoSanitizeAttr>())
     if (Attr->getMask() & SanitizerKind::Address)
       IsBlacklisted = true;
   reportGlobalToASan(GV, D.getLocation(), OS.str(), D.getType(), IsDynInit,

lib/Sema/SemaDeclAttr.cpp

         !(Attr.hasScope() && Attr.getScopeName()->isStr("gnu")))
       S.Diag(Attr.getLoc(), diag::ext_cxx14_attr) << Attr.getName();
 
-  D->addAttr(::new (S.Context)
-                 DeprecatedAttr(Attr.getRange(), S.Context, Str, Replacement,
+  D->addAttr(::new (S.Context) DeprecatedAttr(Attr.getRange(), S.Context,
+                                              Str, Replacement,
                                               Attr.getAttributeSpellingListIndex()));
 }
 
 static bool isGlobalVar(const Decl *D) {

 
     if (parseSanitizerValue(SanitizerName, /*AllowGroups=*/true) == 0)
       S.Diag(LiteralLoc, diag::warn_unknown_sanitizer_ignored) << SanitizerName;
-    else if (isGlobalVar(D) && SanitizerName != "address")
+    else if (isGlobalVar(D) && SanitizerName != "address") {
       S.Diag(D->getLocation(), diag::err_attribute_wrong_decl_type)
           << Attr.getName() << ExpectedFunctionOrMethod;
+      return;
+    }
     Sanitizers.push_back(SanitizerName);
   }
 

                                 .Case("no_sanitize_address", "address")
                                 .Case("no_sanitize_thread", "thread")
                                 .Case("no_sanitize_memory", "memory");
-  if (isGlobalVar(D) && SanitizerName != "address")
+  if (isGlobalVar(D) && SanitizerName != "address") {
     S.Diag(D->getLocation(), diag::err_attribute_wrong_decl_type)
         << Attr.getName() << ExpectedFunction;
+    return;
+  }
   D->addAttr(::new (S.Context)
                  NoSanitizeAttr(Attr.getRange(), S.Context, &SanitizerName, 1,
                                 Attr.getAttributeSpellingListIndex()));

test/CodeGen/asan-globals.cpp

 
 int global;
 int dyn_init_global = global;
-int __attribute__((no_sanitize("address"))) attributed_global;
+int __attribute__((no_sanitize("address"))) attributed_global1;
+int __attribute__((no_sanitize_address)) attributed_global2;
 int blacklisted_global;
 
 void func() {

   const char *literal = "Hello, world!";
 }
 
-// CHECK: !llvm.asan.globals = !{![[EXTRA_GLOBAL:[0-9]+]], ![[GLOBAL:[0-9]+]], ![[DYN_INIT_GLOBAL:[0-9]+]], ![[ATTR_GLOBAL:[0-9]+]], ![[BLACKLISTED_GLOBAL:[0-9]+]], ![[STATIC_VAR:[0-9]+]], ![[LITERAL:[0-9]+]]}
+// CHECK: !llvm.asan.globals = !{![[EXTRA_GLOBAL:[0-9]+]], ![[GLOBAL:[0-9]+]], ![[DYN_INIT_GLOBAL:[0-9]+]], ![[ATTR_GLOBAL1:[0-9]+]], ![[ATTR_GLOBAL2:[0-9]+]], ![[BLACKLISTED_GLOBAL:[0-9]+]], ![[STATIC_VAR:[0-9]+]], ![[LITERAL:[0-9]+]]}
 // CHECK: ![[EXTRA_GLOBAL]] = !{{{.*}} ![[EXTRA_GLOBAL_LOC:[0-9]+]], !"extra_global", i1 false, i1 false}
 // CHECK: ![[EXTRA_GLOBAL_LOC]] = !{!"{{.*}}extra-source.cpp", i32 1, i32 5}
 // CHECK: ![[GLOBAL]] = !{{{.*}} ![[GLOBAL_LOC:[0-9]+]], !"global", i1 false, i1 false}
 // CHECK: ![[GLOBAL_LOC]] = !{!"{{.*}}asan-globals.cpp", i32 8, i32 5}
 // CHECK: ![[DYN_INIT_GLOBAL]] = !{{{.*}} ![[DYN_INIT_LOC:[0-9]+]], !"dyn_init_global", i1 true, i1 false}
 // CHECK: ![[DYN_INIT_LOC]] = !{!"{{.*}}asan-globals.cpp", i32 9, i32 5}
-// CHECK: ![[ATTR_GLOBAL]] = !{{{.*}}, null, null, i1 false, i1 true}
+// CHECK: ![[ATTR_GLOBAL1]] = !{{{.*}}, null, null, i1 false, i1 true}
+// CHECK: ![[ATTR_GLOBAL2]] = !{{{.*}}, null, null, i1 false, i1 true}
 // CHECK: ![[BLACKLISTED_GLOBAL]] = !{{{.*}}, null, null, i1 false, i1 true}
 // CHECK: ![[STATIC_VAR]] = !{{{.*}} ![[STATIC_LOC:[0-9]+]], !"static_var", i1 false, i1 false}
-// CHECK: ![[STATIC_LOC]] = !{!"{{.*}}asan-globals.cpp", i32 14, i32 14}
+// CHECK: ![[STATIC_LOC]] = !{!"{{.*}}asan-globals.cpp", i32 15, i32 14}
 // CHECK: ![[LITERAL]] = !{{{.*}} ![[LITERAL_LOC:[0-9]+]], !"<string literal>", i1 false, i1 false}
-// CHECK: ![[LITERAL_LOC]] = !{!"{{.*}}asan-globals.cpp", i32 15, i32 25}
+// CHECK: ![[LITERAL_LOC]] = !{!"{{.*}}asan-globals.cpp", i32 16, i32 25}
 
-// BLACKLIST-SRC: !llvm.asan.globals = !{![[EXTRA_GLOBAL:[0-9]+]], ![[GLOBAL:[0-9]+]], ![[DYN_INIT_GLOBAL:[0-9]+]], ![[ATTR_GLOBAL:[0-9]+]], ![[BLACKLISTED_GLOBAL:[0-9]+]], ![[STATIC_VAR:[0-9]+]], ![[LITERAL:[0-9]+]]}
+// BLACKLIST-SRC: !llvm.asan.globals = !{![[EXTRA_GLOBAL:[0-9]+]], ![[GLOBAL:[0-9]+]], ![[DYN_INIT_GLOBAL:[0-9]+]], ![[ATTR_GLOBAL1:[0-9]+]], ![[ATTR_GLOBAL2:[0-9]+]], ![[BLACKLISTED_GLOBAL:[0-9]+]], ![[STATIC_VAR:[0-9]+]], ![[LITERAL:[0-9]+]]}
 // BLACKLIST-SRC: ![[EXTRA_GLOBAL]] = !{{{.*}} ![[EXTRA_GLOBAL_LOC:[0-9]+]], !"extra_global", i1 false, i1 false}
 // BLACKLIST-SRC: ![[EXTRA_GLOBAL_LOC]] = !{!"{{.*}}extra-source.cpp", i32 1, i32 5}
 // BLACKLIST-SRC: ![[GLOBAL]] = !{{{.*}} null, null, i1 false, i1 true}
 // BLACKLIST-SRC: ![[DYN_INIT_GLOBAL]] = !{{{.*}} null, null, i1 true, i1 true}
-// BLACKLIST-SRC: ![[ATTR_GLOBAL]] = !{{{.*}}, null, null, i1 false, i1 true}
+// BLACKLIST-SRC: ![[ATTR_GLOBAL1]] = !{{{.*}}, null, null, i1 false, i1 true}
+// BLACKLIST-SRC: ![[ATTR_GLOBAL2]] = !{{{.*}}, null, null, i1 false, i1 true}
 // BLACKLIST-SRC: ![[BLACKLISTED_GLOBAL]] = !{{{.*}}, null, null, i1 false, i1 true}
 // BLACKLIST-SRC: ![[STATIC_VAR]] = !{{{.*}} null, null, i1 false, i1 true}
 // BLACKLIST-SRC: ![[LITERAL]] = !{{{.*}} null, null, i1 false, i1 true}

test/SemaCXX/attr-no-sanitize-address.cpp

 int noanal_testfn(int y) NO_SANITIZE_ADDRESS;
 
 int noanal_testfn(int y) {
+  // Test the deprecated spelling of no_sanitize("address")
   int x NO_SANITIZE_ADDRESS = y; // \
-    // expected-error {{'no_sanitize_address' attribute only applies to functions}}
+    // expected-error {{'no_sanitize_address' attribute only applies to functions and global variables}}
+  int x2 __attribute__((no_sanitize("address"))) = y; // \
+    // expected-error {{'no_sanitize' attribute only applies to functions, methods, and global variables}}
   return x;
 }