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

[abrachet]

Summary

This emits an st_size that represents the actual useable size of an object before the redzone is added.

[MaskRay]

This matches gcc's behavior of keeping an objects st_size as it was before the redzone is added. Keeping the smaller size is beneficial for reducing the amount copied in copy relocations.

This is insufficient justification to me. With compilation for executables moving from -fno-pic to -fpie, we cannot see copy relocations.
Copy relocations really only happen with uncommon configurations now and decreasing st_size doesn't save many bytes any way. (Copy relocations can also be avoided with -fno-direct-access-external-data.)
The enlarged st_size value gives users a quick way to know whether the area has been enlarged.
I do not see why we need to match GCC's behavior here.

[abrachet]

In D123010#3425554, @MaskRay wrote:

The enlarged st_size value gives users a quick way to know whether the area has been enlarged.

In what cases do you imagine it would be useful to quickly see whether an area has been enlarged? If anything this makes abi tracking mechanisms through nm, like what libcxx does, and we plan to do with compiler_rt with llvm-ifs more difficult because symbol sizes differ between clang and gcc. Granted compiler_rt is a bad example here because it cannot be itself sanitized, likewise libcxx doesn't track object sizes, however I think the point is still valid.

This is insufficient justification to me. With compilation for executables moving from -fno-pic to -fpie, we cannot see copy relocations.
Copy relocations really only happen with uncommon configurations now and decreasing st_size doesn't save many bytes any way. (Copy relocations can also be avoided with -fno-direct-access-external-data.)

Sure, I think regardless of rarity of the use-case it is objectively better to make copy relocations smaller for when they are used.

I do not see why we need to match GCC's behavior here.

Matching GCC when their behavior is better seems like a worthy goal, especially if the cost to do so here isn't high. Do you expect the complexity of whatever solution this would end up with isn't worth the benefit?

[MaskRay]

It is more like: "we do not care much about which st_size to use, but I prefer the way with little complexity".

This patch tries to change the status quo with a bit more complexity, so I'd challenge it with the justification.
Your llvm-ifs argument in the comment would make a better summary, but see below I have a minor concern with regard to copy relocations.
In addition, asan instruments global variables so the ABI is different anyway. I do not see a very justification that unifying the two cases.

In what cases do you imagine it would be useful to quickly see whether an area has been enlarged?

Mixing instrumented and uninstrumented global variables are error-prone, so knowing whether some have actually different IR sizes helps debugging if a program breaks.
Also imagine the scenario that asan improves to place some non-zero data into the trailing redzone, then this approach will actually break copy relocations.

Matching GCC when their behavior is better seems like a worthy goal, especially if the cost to do so here isn't high. Do you expect the complexity of whatever solution this would end up with isn't worth the benefit?

I believe in many of the sanitizer cases Clang has the reference implementation and GCC is following.
In such cases changes need to have their own merits, the GCC compatibility is just a tie-breaker.
Actually I have some vague memory that GCC hasn't thought hard about what st_size it should use, as I vaguely remember some global variable instrumentation in GCC.

[abrachet]

In D123010#3428517, @MaskRay wrote:

It is more like: "we do not care much about which st_size to use, but I prefer the way with little complexity".

I've updated this patch with a more permanent solution. I think how it is now, the complexity is not a large concern. WDYT?

This patch tries to change the status quo with a bit more complexity, so I'd challenge it with the justification.
Your llvm-ifs argument in the comment would make a better summary, but see below I have a minor concern with regard to copy relocations.
In addition, asan instruments global variables so the ABI is different anyway. I do not see a very justification that unifying the two cases.

I think in this case it is important that llvm-ifs report the same abi when building with either gcc or clang. These abi checking tools would already report an abi difference between instrumented binaries because of the addition of the __asan_* symbols.

In what cases do you imagine it would be useful to quickly see whether an area has been enlarged?

Mixing instrumented and uninstrumented global variables are error-prone, so knowing whether some have actually different IR sizes helps debugging if a program breaks.
Also imagine the scenario that asan improves to place some non-zero data into the trailing redzone, then this approach will actually break copy relocations.

This is true. But I don't think the potential for future work should block this. That would be a large overhaul to asan, it could easily remove the 4 lines that emit the explicit_size metadata tag when it's semantics change.

[MaskRay]

Thinking this again, I think using the original st_size makes sense, but I do not agree with the justification put on the patch.
The original st_size makes sense because the size is the inherent property of the global variable and should not change due to error-catching instrumentation.
I.e. if the size 4 is grown to 6, the user application can only access [0,4), not [4,6).

Matching GCC behavior is very minor pros in this case (depending on cases, it may matter more). The copy relocations benefit is even minor and not worthwhile to be mentioned in LangRef.

[abrachet]

Update docs and commit message

[MaskRay]

Mostly fine to me.

I cannot think of a GlobalMerge similar optimization which may need adaptation. (!associated doesn't seem to have any)

llvm/docs/LangRef.rst

7100

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.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

694

Such IIFE is not common. Why not just remove the lambda?

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

2516

More idiom way is something like
createConstant(ConstantInt::get(Type::getInt64Ty(C), SizeInBytes)))

llvm/test/CodeGen/X86/explicit-size-metadata.ll

7

Test an array

llvm/test/Instrumentation/AddressSanitizer/adaptive_global_redzones.ll

55–56

Update the two CHECK lines as well.

[efriedma]

This seems really obscure, but makes sense.

llvm/docs/LangRef.rst

7100

Probably makes sense to diagnose on anything that isn't a definition of a variable. Probably not worth trying to catch other dubious cases.

Maybe explicitly note that this doesn't make sense for all object formats?

[abrachet]

Address comments

[abrachet]

llvm/docs/LangRef.rst

7100

Thanks @MaskRay I've used this comment verbatim. Plus added some about how this is only meaningful on ELF.

Probably makes sense to diagnose on anything that isn't a definition of a variable.

How do you reckon I would do this? It seems like the current behavior is to just ignore metadata that doesn't get used. Seems difficult to warn about places where !explicit_size isn't meaningful. Do you know of an example where this happens in llvm currently I could look at?

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

694

Sure. I was thinking it would be too many nested if's. But I guess it looks fine.

[efriedma]

llvm/docs/LangRef.rst

7100

It would be pretty easy to add something to Verifier::visitGlobalValue. I guess we don't actually do much verification for most metadata at the moment, though. But see Verifier::verifyFunctionMetadata.

[abrachet]

Verify !explicit_size on GlobalValues

[MaskRay]

llvm/test/Verifier/explicit_size.ll

3

This directory doesn't use llc. You can verify by looking for other tests.

Use opt -verify -disable-output

10

As mentioned, an aggregate type makes more sense and should be tested

[abrachet]

llvm/test/Verifier/explicit_size.ll

3

Thanks. Sorry about that.

[MaskRay]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

697

Use cast which asserts non-null.

701

Use cast which asserts non-null.

llvm/test/CodeGen/X86/explicit-size-metadata.ll

2

Try making tests more general by checking the generic ELF behavior, not linux-gnu specific ones.

llvm/test/Instrumentation/AddressSanitizer/adaptive_global_redzones.ll

9

![[#TAG10:]] (match a decimal)

Reference it below with [[#TAG10]]

This is better than .* which matches too much, and {{[0-9]+}} which is too verbose.

55–56

![[#]]

[MaskRay]

Please wait for a sanitizer reviewer (@vitalybuka) to confirm this is fine.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

695

Omit this: getOperand(0) has an assert

[abrachet]

Remove assert

[vitalybuka]

LGTM
@hctim is in a middle of asan globals refactoring, maybe he can spot something unusual.

[hctim]

I just did a quick check against llvm/test/Instrumentation/AddressSanitizer/adaptive_global_redzones.ll, compiling and linking it as a DSO, and it looks right (only the sizes change).

One thing to bear in mind is that some sanitizers (like HWASan) also change the alignment of the global variables as well as the size, so sanitizers will always be an ABI break, but I agree this patch makes things less confusing rather than more confusing.

[vitalybuka]

This change breaks a lot of our internal asan tests. I am going to revert for investigation.

[abrachet]

Hi folks wanted to update here why this was breaking.

When we decreased the size non-pie's needed a copy relocation, part of the intention here was to make them smaller. In these cases the linker would have only created enough space based on the st_size of the object. Which just became too small. The caveat here, is alignment if you have sufficient alignment you may have enough space for the object and it's redzone. This is of course not guaranteed, except for small objects where the alignment is likely to fit both. What ended up happening was objects being placed too close together and being in the redzone of the previous object.

As for gcc's implementation, they use the smaller st_size, the difference is the array they pass to __asan_register_globals is a .Lsymbol, generating relative relocations to the globals this is as if you passed -mllvm -asan-use-private-alias to clang. In the non-pie case, gcc is registering the global in the shared object which got preempted by the one in the executable and poisoning nothing. I doubt this behavior is intended on their end.

llvm/docs/LangRef.rst

 
 Example:
 
 .. code-block:: text
 
     %a.addr = alloca float*, align 8, !annotation !0
     !0 = !{!"auto-init"}
 
+'``explicit_size``' Metadata
+^^^^^^^^^^^^^^^^^^^^^^^^^
+
+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. This is currently only meaningful
+on ELF where an objects ``st_size`` can be controlled with a ``.size``
+directive. This metadata is ignored in contexts where it cannot be honored.
+
 Module Flags Metadata
 =====================
 
 Information about the module as a whole is difficult to convey to LLVM's
 subsystems. The LLVM IR isn't sufficient to transmit this information.
 The ``llvm.module.flags`` named metadata exists in order to facilitate
 this. These flags are in the form of key / value pairs --- much like a
 dictionary --- making it easy for any subsystem who cares about a flag to

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

void AsmPrinter::emitGlobalVariable(const GlobalVariable *GV) {
 
   OutStreamer->emitLabel(EmittedInitSym);
   MCSymbol *LocalAlias = getSymbolPreferLocal(*GV);
   if (LocalAlias != EmittedInitSym)
     OutStreamer->emitLabel(LocalAlias);
 
   emitGlobalConstant(GV->getParent()->getDataLayout(), GV->getInitializer());
 
-  if (MAI->hasDotTypeDotSizeDirective())
+  if (MAI->hasDotTypeDotSizeDirective()) {
+    if (const MDNode *ExplicitValue = GV->getMetadata("explicit_size")) {
+      assert(ExplicitValue->getNumOperands() &&
+             "Should have been checked by verifier allready");
+      const auto *MetadataValue =
+          cast<ValueAsMetadata>(ExplicitValue->getOperand(0));
+      const auto *CI = cast<ConstantInt>(MetadataValue->getValue());
+      Size = CI->getZExtValue();
+    }
     // .size foo, 42
     OutStreamer->emitELFSize(EmittedInitSym,
                              MCConstantExpr::create(Size, OutContext));
+  }
 
   OutStreamer->AddBlankLine();
 }
 
 /// Emit the directive and value for debug thread local expression
 ///
 /// \p Value - The value to emit.
 /// \p Size - The size of the integer (in bytes) to emit.

llvm/lib/IR/Verifier.cpp

void Verifier::visitGlobalVariable(const GlobalVariable &GV) {
   for (auto *MD : MDs) {
     if (auto *GVE = dyn_cast<DIGlobalVariableExpression>(MD))
       visitDIGlobalVariableExpression(*GVE);
     else
       CheckDI(false, "!dbg attachment of global variable must be a "
                      "DIGlobalVariableExpression");
   }
 
+  MDs.clear();
+  GV.getMetadata("explicit_size", MDs);
+  if (MDs.size()) {
+    Check(MDs.size() == 1,
+          "only one !explicit_size can be attached to a global variable");
+    Check(MDs[0]->getNumOperands() == 1,
+          "!explicit_size must have exactly one operand");
+    const auto *MetadataValue =
+        dyn_cast<ValueAsMetadata>(MDs[0]->getOperand(0));
+    Check(MetadataValue, "!explicit_size operand must be a value");
+    const auto *CI = dyn_cast<ConstantInt>(MetadataValue->getValue());
+    Check(CI, "!explicit_size value must be an integer");
+  }
+
   // Scalable vectors cannot be global variables, since we don't know
   // the runtime size. If the global is an array containing scalable vectors,
   // that will be caught by the isValidElementType methods in StructType or
   // ArrayType instead.
   Check(!isa<ScalableVectorType>(GV.getValueType()),
         "Globals cannot contain scalable vectors", &GV);
 
   if (auto *STy = dyn_cast<StructType>(GV.getValueType()))

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

if (TargetTriple.isOSBinFormatMachO() && !G->hasSection() &&
       if (Seq && Seq->isCString())
         NewGlobal->setSection("__TEXT,__asan_cstring,regular");
     }
 
     // Transfer the debug info and type metadata.  The payload starts at offset
     // zero so we can copy the metadata over as is.
     NewGlobal->copyMetadata(G, 0);
 
+    LLVMContext &C = NewGlobal->getContext();
+    MDBuilder MDB{C};
+    MDTuple *MDN = MDNode::get(C, MDB.createConstant(ConstantInt::get(
+                                      Type::getInt64Ty(C), SizeInBytes)));
+    NewGlobal->setMetadata("explicit_size", MDN);
+
     Value *Indices2[2];
     Indices2[0] = IRB.getInt32(0);
     Indices2[1] = IRB.getInt32(0);
 
     G->replaceAllUsesWith(
         ConstantExpr::getGetElementPtr(NewTy, NewGlobal, Indices2, true));
     NewGlobal->takeName(G);
     G->eraseFromParent();

llvm/test/CodeGen/X86/explicit-size-metadata.ll

+; RUN: llc < %s -mtriple=x86_64 | FileCheck %s
+
+@a = global i64 0, align 8, !explicit_size !0
+; CHECK: .size a, 4
+
+@b = global i64 0, align 8
+; CHECK: .size b, 8
+
+@larger = global i16 0, align 4, !explicit_size !0
+; CHECK: .size larger, 4
+
+@array = global { [8 x i8] } zeroinitializer, !explicit_size !0
+; CHECK: .size array, 4
+
+!0 = !{i64 4}

llvm/test/Instrumentation/AddressSanitizer/adaptive_global_redzones.ll

 ; RUN: opt < %s -passes='asan-pipeline' -S | FileCheck %s
 ; RUN: opt < %s -passes='asan-pipeline' -asan-mapping-scale=5 -S | FileCheck %s
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
 target triple = "x86_64-unknown-linux-gnu"
 
 ; Here we check that the global redzone sizes grow with the object size.
 
 @G10 = global [10 x i8] zeroinitializer, align 1
-; CHECK: @G10 = global { [10 x i8], [22 x i8] }
+; CHECK: @G10 = global { [10 x i8], [22 x i8] } zeroinitializer, align 32, !explicit_size ![[#TAG10:]]
 
 @G31 = global [31 x i8] zeroinitializer, align 1
 @G32 = global [32 x i8] zeroinitializer, align 1
 @G33 = global [33 x i8] zeroinitializer, align 1
-; CHECK: @G31 = global { [31 x i8], [33 x i8] }
-; CHECK: @G32 = global { [32 x i8], [32 x i8] }
-; CHECK: @G33 = global { [33 x i8], [63 x i8] }
+; CHECK: @G31 = global { [31 x i8], [33 x i8] } zeroinitializer, align 32, !explicit_size ![[#TAG31:]]
+; CHECK: @G32 = global { [32 x i8], [32 x i8] } zeroinitializer, align 32, !explicit_size ![[#TAG32:]]
+; CHECK: @G33 = global { [33 x i8], [63 x i8] } zeroinitializer, align 32, !explicit_size ![[#TAG33:]]
 
 @G63 = global [63 x i8] zeroinitializer, align 1
 @G64 = global [64 x i8] zeroinitializer, align 1
 @G65 = global [65 x i8] zeroinitializer, align 1
 ; CHECK: @G63 = global { [63 x i8], [33 x i8] }
 ; CHECK: @G64 = global { [64 x i8], [32 x i8] }
 ; CHECK: @G65 = global { [65 x i8], [63 x i8] }
 

 @G1024 = global [1024 x i8] zeroinitializer, align 1
 @G1025 = global [1025 x i8] zeroinitializer, align 1
 ; CHECK: @G1023 = global { [1023 x i8], [225 x i8] }
 ; CHECK: @G1024 = global { [1024 x i8], [256 x i8] }
 ; CHECK: @G1025 = global { [1025 x i8], [287 x i8] }
 
 @G1000000 = global [1000000 x i8] zeroinitializer, align 1
 @G10000000 = global [10000000 x i8] zeroinitializer, align 1
 @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] }
-; CHECK: @G100000000 = global { [100000000 x i8], [262144 x i8] }
+; CHECK: @G1000000 = global { [1000000 x i8], [249984 x i8] } zeroinitializer, align 32, !explicit_size ![[#]]
+; CHECK: @G10000000 = global { [10000000 x i8], [262144 x i8] } zeroinitializer, align 32, !explicit_size ![[#]]
+; CHECK: @G100000000 = global { [100000000 x i8], [262144 x i8] } zeroinitializer, align 32, !explicit_size ![[#TAGBIG:]]
+
+; CHECK: ![[#TAG10]] = !{i64 10}
+; CHECK: ![[#TAG31]] = !{i64 31}
+; CHECK: ![[#TAG32]] = !{i64 32}
+; CHECK: ![[#TAG33]] = !{i64 33}
+; CHECK: ![[#TAGBIG]] = !{i64 100000000}

llvm/test/Instrumentation/AddressSanitizer/debug-info-global-var.ll

 ; RUN: opt < %s -passes='asan-pipeline' -S | FileCheck %s
 source_filename = "version.c"
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.12.0"
-; CHECK: @version = constant { [5 x i8], [27 x i8] } {{.*}}, !dbg ![[GV:.*]]
+; CHECK: @version = constant { [5 x i8], [27 x i8] } {{.*}}, !dbg ![[GV:.*]],
 
 @version = constant [5 x i8] c"4.00\00", align 1, !dbg !0
 
 !llvm.dbg.cu = !{!2}
 !llvm.module.flags = !{!11, !12, !13}
 !llvm.ident = !{!14}
 ; Should not have an expression:
 ; CHECK: ![[GV]] = !DIGlobalVariableExpression(var: ![[GVAR:.*]], expr: !DIExpression())

llvm/test/Verifier/explicit_size.ll

+; RUN: split-file %s %t
+; RUN: not opt -verify -disable-output < %t/multiple.ll 2>&1 | FileCheck %s --check-prefix=MULTIPLE
+; RUN: not opt -verify -disable-output < %t/operands.ll 2>&1 | FileCheck %s --check-prefix=OPERANDS
+; RUN: not opt -verify -disable-output < %t/invalid_md.ll 2>&1 | FileCheck %s --check-prefix=INVALID_MD
+; RUN: not opt -verify -disable-output < %t/not_int.ll 2>&1 | FileCheck %s --check-prefix=NOT_INT
+
+; MULTIPLE: only one !explicit_size can be attached to a global variable
+;--- multiple.ll
+@a = global { i32, [28 x i8] } zeroinitializer, align 32, !explicit_size !0, !explicit_size !0
+
+!0 = !{i64 4}
+
+; OPERANDS: !explicit_size must have exactly one operand
+;--- operands.ll
+@a = global { i32, [28 x i8] } zeroinitializer, align 32, !explicit_size !0
+
+!0 = !{i64 4, i64 4}
+
+; INVALID_MD: !explicit_size operand must be a value
+;--- invalid_md.ll
+@a = global { i32, [28 x i8] } zeroinitializer, align 32, !explicit_size !0
+
+!0 = !{!1}
+!1 = !{i64 4}
+
+; NOT_INT: !explicit_size value must be an integer
+;--- not_int.ll
+@a = global { i32, [28 x i8] } zeroinitializer, align 32, !explicit_size !0
+declare i32 @b()
+
+!0 = !{i32 ()* @b}