r296374 - [ubsan] Factor out logic to emit a range check. NFC.

[Vedant Kumar via cfe-commits]

Hi Ivan,

I saw a bot failure on your job after this commit:

  http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/5467/steps/tsan%20analyze/logs/stdio
  http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/5467/steps/build%20release%20tsan%20with%20clang/logs/stdio

However, I cannot reproduce it locally with a stage2 TSAN build.

After staring at my diff I couldn't find anything that would explain the failure. No other bots seem upset.

Do you have any idea about what's going on? Let me know if you want me to revert...

vedant

> On Feb 27, 2017, at 11:46 AM, Vedant Kumar via cfe-commits <cfe-commits at lists.llvm.org> wrote:
> 
> Author: vedantk
> Date: Mon Feb 27 13:46:19 2017
> New Revision: 296374
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=296374&view=rev
> Log:
> [ubsan] Factor out logic to emit a range check. NFC.
> 
> This is a readability improvement, but it will also help prep an
> upcoming patch to detect UB loads from bitfields.
> 
> Modified:
>    cfe/trunk/lib/CodeGen/CGExpr.cpp
>    cfe/trunk/lib/CodeGen/CodeGenFunction.h
> 
> Modified: cfe/trunk/lib/CodeGen/CGExpr.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExpr.cpp?rev=296374&r1=296373&r2=296374&view=diff
> ==============================================================================
> --- cfe/trunk/lib/CodeGen/CGExpr.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGExpr.cpp Mon Feb 27 13:46:19 2017
> @@ -1301,6 +1301,46 @@ llvm::MDNode *CodeGenFunction::getRangeF
>   return MDHelper.createRange(Min, End);
> }
> 
> +bool CodeGenFunction::EmitScalarRangeCheck(llvm::Value *Value, QualType Ty,
> +                                           SourceLocation Loc) {
> +  bool HasBoolCheck = SanOpts.has(SanitizerKind::Bool);
> +  bool HasEnumCheck = SanOpts.has(SanitizerKind::Enum);
> +  if (!HasBoolCheck && !HasEnumCheck)
> +    return false;
> +
> +  bool IsBool = hasBooleanRepresentation(Ty) ||
> +                NSAPI(CGM.getContext()).isObjCBOOLType(Ty);
> +  bool NeedsBoolCheck = HasBoolCheck && IsBool;
> +  bool NeedsEnumCheck = HasEnumCheck && Ty->getAs<EnumType>();
> +  if (!NeedsBoolCheck && !NeedsEnumCheck)
> +    return false;
> +
> +  llvm::APInt Min, End;
> +  if (!getRangeForType(*this, Ty, Min, End, /*StrictEnums=*/true, IsBool))
> +    return true;
> +
> +  SanitizerScope SanScope(this);
> +  llvm::Value *Check;
> +  --End;
> +  if (!Min) {
> +    Check = Builder.CreateICmpULE(
> +        Value, llvm::ConstantInt::get(getLLVMContext(), End));
> +  } else {
> +    llvm::Value *Upper = Builder.CreateICmpSLE(
> +        Value, llvm::ConstantInt::get(getLLVMContext(), End));
> +    llvm::Value *Lower = Builder.CreateICmpSGE(
> +        Value, llvm::ConstantInt::get(getLLVMContext(), Min));
> +    Check = Builder.CreateAnd(Upper, Lower);
> +  }
> +  llvm::Constant *StaticArgs[] = {EmitCheckSourceLocation(Loc),
> +                                  EmitCheckTypeDescriptor(Ty)};
> +  SanitizerMask Kind =
> +      NeedsEnumCheck ? SanitizerKind::Enum : SanitizerKind::Bool;
> +  EmitCheck(std::make_pair(Check, Kind), SanitizerHandler::LoadInvalidValue,
> +            StaticArgs, EmitCheckValue(Value));
> +  return true;
> +}
> +
> llvm::Value *CodeGenFunction::EmitLoadOfScalar(Address Addr, bool Volatile,
>                                                QualType Ty,
>                                                SourceLocation Loc,
> @@ -1353,35 +1393,9 @@ llvm::Value *CodeGenFunction::EmitLoadOf
>                                       false /*ConvertTypeToTag*/);
>   }
> 
> -  bool IsBool = hasBooleanRepresentation(Ty) ||
> -                NSAPI(CGM.getContext()).isObjCBOOLType(Ty);
> -  bool NeedsBoolCheck = SanOpts.has(SanitizerKind::Bool) && IsBool;
> -  bool NeedsEnumCheck =
> -      SanOpts.has(SanitizerKind::Enum) && Ty->getAs<EnumType>();
> -  if (NeedsBoolCheck || NeedsEnumCheck) {
> -    SanitizerScope SanScope(this);
> -    llvm::APInt Min, End;
> -    if (getRangeForType(*this, Ty, Min, End, /*StrictEnums=*/true, IsBool)) {
> -      --End;
> -      llvm::Value *Check;
> -      if (!Min)
> -        Check = Builder.CreateICmpULE(
> -          Load, llvm::ConstantInt::get(getLLVMContext(), End));
> -      else {
> -        llvm::Value *Upper = Builder.CreateICmpSLE(
> -          Load, llvm::ConstantInt::get(getLLVMContext(), End));
> -        llvm::Value *Lower = Builder.CreateICmpSGE(
> -          Load, llvm::ConstantInt::get(getLLVMContext(), Min));
> -        Check = Builder.CreateAnd(Upper, Lower);
> -      }
> -      llvm::Constant *StaticArgs[] = {
> -        EmitCheckSourceLocation(Loc),
> -        EmitCheckTypeDescriptor(Ty)
> -      };
> -      SanitizerMask Kind = NeedsEnumCheck ? SanitizerKind::Enum : SanitizerKind::Bool;
> -      EmitCheck(std::make_pair(Check, Kind), SanitizerHandler::LoadInvalidValue,
> -                StaticArgs, EmitCheckValue(Load));
> -    }
> +  if (EmitScalarRangeCheck(Load, Ty, Loc)) {
> +    // In order to prevent the optimizer from throwing away the check, don't
> +    // attach range metadata to the load.
>   } else if (CGM.getCodeGenOpts().OptimizationLevel > 0)
>     if (llvm::MDNode *RangeInfo = getRangeForLoadFromType(Ty))
>       Load->setMetadata(llvm::LLVMContext::MD_range, RangeInfo);
> 
> Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.h?rev=296374&r1=296373&r2=296374&view=diff
> ==============================================================================
> --- cfe/trunk/lib/CodeGen/CodeGenFunction.h (original)
> +++ cfe/trunk/lib/CodeGen/CodeGenFunction.h Mon Feb 27 13:46:19 2017
> @@ -2866,6 +2866,13 @@ public:
>   /// representation to its value representation.
>   llvm::Value *EmitFromMemory(llvm::Value *Value, QualType Ty);
> 
> +  /// Check if the scalar \p Value is within the valid range for the given
> +  /// type \p Ty.
> +  ///
> +  /// Returns true if a check is needed (even if the range is unknown).
> +  bool EmitScalarRangeCheck(llvm::Value *Value, QualType Ty,
> +                            SourceLocation Loc);
> +
>   /// EmitLoadOfScalar - Load a scalar value from an address, taking
>   /// care to appropriately convert from the memory representation to
>   /// the LLVM value representation.
> 
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits