[PATCH] D123010: [asan] Emit .size directive for global object size before redzone

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Mostly fine to me.



================
Comment at: llvm/docs/LangRef.rst:7098
+
+The ``explicit_size`` metadata is used to emit a size directive with a different
+size than the objects total size. This can be useful when an object contains
----------------
Suggest: The ``explicit_size`` metadata  may be attached to a global variable definition with a size different from the object's total size. This can be useful when an instrumentation enlarges the object while the symbol size should reflect the accessible or meaningful part of the object.


I think this is not meaningful to functions, ifuncs, comdats, etc, and likely not meaningful to scalar types, but it may not be necessary to catch the error in the IR verifier.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:694
+  if (MAI->hasDotTypeDotSizeDirective()) {
+    [&Size, GV] {
+      const MDNode *ExplicitValue = GV->getMetadata("explicit_size");
----------------
Such IIFE is not common. Why not just remove the lambda?


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:2516
+    MDNode *N = MDNode::get(C, ConstantAsMetadata::get(Int));
+    NewGlobal->setMetadata("explicit_size", N);
+
----------------
More idiom way is something like 
`createConstant(ConstantInt::get(Type::getInt64Ty(C), SizeInBytes)))`


================
Comment at: llvm/test/CodeGen/X86/explicit-size-metadata.ll:6
+
+ at b = dso_local global i64 0, align 8
+; CHECK: .size b, 8
----------------
Test an array


================
Comment at: llvm/test/Instrumentation/AddressSanitizer/adaptive_global_redzones.ll:56
 @G100000000 = global [100000000 x i8] zeroinitializer, align 1
 ; CHECK: @G1000000 = global { [1000000 x i8], [249984 x i8] }
 ; CHECK: @G10000000 = global { [10000000 x i8], [262144 x i8] }
----------------
Update the two CHECK lines as well.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123010/new/

https://reviews.llvm.org/D123010