From 31a8bf1e251bdc3d6e74a4fded7d95c6e274b5d8 Mon Sep 17 00:00:00 2001 From: Nuno Lopes Date: Sun, 28 Apr 2024 12:42:47 +0100 Subject: [PATCH] fix #1028: the align attribute on load, store, etc implies dereferenceability We had a partial implementation for the ASM mode; this patch superseeds that one --- ir/instr.cpp | 8 ++--- ir/memory.cpp | 3 +- ir/pointer.cpp | 6 ++-- smt/expr.cpp | 5 +++ smt/expr.h | 2 ++ .../align-callsite-withnoundef.srctgt.ll | 2 ++ .../align-callsite2-withnoundef.srctgt.ll | 2 ++ .../align-callsite3-withnoundef.srctgt.ll | 2 ++ .../align-callsite4-withnoundef.srctgt.ll | 13 +++++++ .../dereferenceable-fndef-heap3.srctgt.ll | 4 +-- tests/alive-tv/bugs/pr3720.srctgt.ll | 16 +++------ tests/alive-tv/memory/align.src.ll | 2 ++ tests/alive-tv/memory/nullptr-align.srctgt.ll | 2 ++ tools/transform.cpp | 36 ++----------------- 14 files changed, 48 insertions(+), 55 deletions(-) create mode 100644 tests/alive-tv/attrs/align-callsite4-withnoundef.srctgt.ll diff --git a/ir/instr.cpp b/ir/instr.cpp index c2eb9cab8..19a722b5a 100644 --- a/ir/instr.cpp +++ b/ir/instr.cpp @@ -3893,7 +3893,7 @@ DEFINE_AS_RETZEROALIGN(Load, getMaxAllocSize); DEFINE_AS_RETZERO(Load, getMaxGEPOffset); uint64_t Load::getMaxAccessSize() const { - return Memory::getStoreByteSize(getType()); + return round_up(Memory::getStoreByteSize(getType()), align); } MemInstr::ByteAccessInfo Load::getByteAccessInfo() const { @@ -3939,7 +3939,7 @@ DEFINE_AS_RETZEROALIGN(Store, getMaxAllocSize); DEFINE_AS_RETZERO(Store, getMaxGEPOffset); uint64_t Store::getMaxAccessSize() const { - return Memory::getStoreByteSize(val->getType()); + return round_up(Memory::getStoreByteSize(val->getType()), align); } MemInstr::ByteAccessInfo Store::getByteAccessInfo() const { @@ -3992,7 +3992,7 @@ DEFINE_AS_RETZEROALIGN(Memset, getMaxAllocSize); DEFINE_AS_RETZERO(Memset, getMaxGEPOffset); uint64_t Memset::getMaxAccessSize() const { - return getIntOr(*bytes, UINT64_MAX); + return round_up(getIntOr(*bytes, UINT64_MAX), align); } MemInstr::ByteAccessInfo Memset::getByteAccessInfo() const { @@ -4160,7 +4160,7 @@ DEFINE_AS_RETZEROALIGN(Memcpy, getMaxAllocSize); DEFINE_AS_RETZERO(Memcpy, getMaxGEPOffset); uint64_t Memcpy::getMaxAccessSize() const { - return getIntOr(*bytes, UINT64_MAX); + return round_up(getIntOr(*bytes, UINT64_MAX), max(align_src, align_dst)); } MemInstr::ByteAccessInfo Memcpy::getByteAccessInfo() const { diff --git a/ir/memory.cpp b/ir/memory.cpp index 7af9170c8..930543bed 100644 --- a/ir/memory.cpp +++ b/ir/memory.cpp @@ -1783,7 +1783,8 @@ Memory::alloc(const expr *size, uint64_t align, BlockKind blockKind, expr size_zext; expr nooverflow = true; if (size) { - size_zext = size->zextOrTrunc(bits_size_t); + size_zext = size->zextOrTrunc(bits_size_t) + .round_up(expr::mkUInt(align, bits_size_t)); nooverflow = size->bits() <= bits_size_t ? true : size->extract(size->bits()-1, bits_size_t) == 0; } diff --git a/ir/pointer.cpp b/ir/pointer.cpp index 4d67de598..9b029790c 100644 --- a/ir/pointer.cpp +++ b/ir/pointer.cpp @@ -428,8 +428,10 @@ static pair is_dereferenceable(Pointer &p, pair Pointer::isDereferenceable(const expr &bytes0, uint64_t align, bool iswrite, bool ignore_accessability) { - expr bytes_off = bytes0.zextOrTrunc(bits_for_offset); - expr bytes = bytes0.zextOrTrunc(bits_size_t); + expr bytes = bytes0.zextOrTrunc(bits_size_t) + .round_up(expr::mkUInt(align, bits_size_t)); + expr bytes_off = bytes.zextOrTrunc(bits_for_offset); + DisjointExpr UB(expr(false)), is_aligned(expr(false)), all_ptrs; for (auto &[ptr_expr, domain] : DisjointExpr(p, 3)) { diff --git a/smt/expr.cpp b/smt/expr.cpp index 5abe5258e..0b2594ff7 100644 --- a/smt/expr.cpp +++ b/smt/expr.cpp @@ -1117,6 +1117,11 @@ expr expr::abs() const { return mkIf(sge(mkUInt(0, s)), *this, mkInt(-1, s) * *this); } +expr expr::round_up(const expr &power_of_two) const { + expr minus_1 = power_of_two - mkUInt(1, power_of_two); + return (*this + minus_1) & ~minus_1; +} + #define fold_fp_neg(fn) \ do { \ expr cond, neg, v, v2; \ diff --git a/smt/expr.h b/smt/expr.h index ebf5d7655..27f521da1 100644 --- a/smt/expr.h +++ b/smt/expr.h @@ -218,6 +218,8 @@ class expr { expr abs() const; + expr round_up(const expr &power_of_two) const; + expr isNaN() const; expr isInf() const; expr isFPZero() const; diff --git a/tests/alive-tv/attrs/align-callsite-withnoundef.srctgt.ll b/tests/alive-tv/attrs/align-callsite-withnoundef.srctgt.ll index 84082fc3b..800e4ef7b 100644 --- a/tests/alive-tv/attrs/align-callsite-withnoundef.srctgt.ll +++ b/tests/alive-tv/attrs/align-callsite-withnoundef.srctgt.ll @@ -1,3 +1,5 @@ +; ERROR: Source is more defined than target + define void @src(ptr %p) { call void @g(ptr %p) load i8, ptr %p diff --git a/tests/alive-tv/attrs/align-callsite2-withnoundef.srctgt.ll b/tests/alive-tv/attrs/align-callsite2-withnoundef.srctgt.ll index 9a615c5ef..c876809d7 100644 --- a/tests/alive-tv/attrs/align-callsite2-withnoundef.srctgt.ll +++ b/tests/alive-tv/attrs/align-callsite2-withnoundef.srctgt.ll @@ -1,3 +1,5 @@ +; ERROR: Source is more defined than target + define void @src(ptr %p) { call void @g(ptr align(4) %p) load i8, ptr %p diff --git a/tests/alive-tv/attrs/align-callsite3-withnoundef.srctgt.ll b/tests/alive-tv/attrs/align-callsite3-withnoundef.srctgt.ll index 3bd526c9e..5d747f608 100644 --- a/tests/alive-tv/attrs/align-callsite3-withnoundef.srctgt.ll +++ b/tests/alive-tv/attrs/align-callsite3-withnoundef.srctgt.ll @@ -1,3 +1,5 @@ +; ERROR: Source is more defined than target + define void @src(ptr %p) { call void @g(ptr align(4) %p) load i8, ptr %p diff --git a/tests/alive-tv/attrs/align-callsite4-withnoundef.srctgt.ll b/tests/alive-tv/attrs/align-callsite4-withnoundef.srctgt.ll new file mode 100644 index 000000000..45a13bc0f --- /dev/null +++ b/tests/alive-tv/attrs/align-callsite4-withnoundef.srctgt.ll @@ -0,0 +1,13 @@ +define void @src(ptr %p) { + call void @g(ptr %p) + load i8, ptr %p + ret void +} + +define void @tgt(ptr %p) { + call void @g(ptr %p) + load i8, ptr %p, align 4 + ret void +} + +declare void @g(ptr align(4) dereferenceable(4) noundef) diff --git a/tests/alive-tv/attrs/dereferenceable-fndef-heap3.srctgt.ll b/tests/alive-tv/attrs/dereferenceable-fndef-heap3.srctgt.ll index 91668c533..05cffd590 100644 --- a/tests/alive-tv/attrs/dereferenceable-fndef-heap3.srctgt.ll +++ b/tests/alive-tv/attrs/dereferenceable-fndef-heap3.srctgt.ll @@ -1,9 +1,9 @@ -define dereferenceable(4) ptr @src() { +define dereferenceable(90) ptr @src() { %p = call ptr @malloc(i64 3) ret ptr %p } -define dereferenceable(4) ptr @tgt() { +define dereferenceable(90) ptr @tgt() { unreachable } diff --git a/tests/alive-tv/bugs/pr3720.srctgt.ll b/tests/alive-tv/bugs/pr3720.srctgt.ll index c948212fe..d26b2ac0f 100644 --- a/tests/alive-tv/bugs/pr3720.srctgt.ll +++ b/tests/alive-tv/bugs/pr3720.srctgt.ll @@ -1,22 +1,14 @@ -%struct.st = type <{ i16 }> - define void @src(ptr %p) { -entry: - %s = alloca %struct.st, align 4 - %0 = getelementptr %struct.st, ptr %s, i32 0, i32 0 - store i16 1, ptr %0, align 4 - %s1 = bitcast ptr %s to ptr - call void @llvm.memcpy.i32(ptr %p, ptr %s1, i32 2, i32 1) + %s = alloca i16, align 4 + store i16 1, ptr %s + call void @llvm.memcpy.i32(ptr %p, ptr %s, i32 2, i32 1) ret void } declare void @llvm.memcpy.i32(ptr nocapture, ptr nocapture, i32, i32) define void @tgt(ptr %p) { -entry: - %p1 = bitcast ptr %p to ptr - %p1.0 = getelementptr %struct.st, ptr %p1, i32 0, i32 0 - store i16 1, ptr %p1.0 + store i16 1, ptr %p ret void } diff --git a/tests/alive-tv/memory/align.src.ll b/tests/alive-tv/memory/align.src.ll index 0d4ce5769..54342cb70 100644 --- a/tests/alive-tv/memory/align.src.ll +++ b/tests/alive-tv/memory/align.src.ll @@ -1,3 +1,5 @@ +; ERROR: Source is more defined than target + define i32 @foo1(ptr %a) { %v = load i32, ptr %a, align 4 %ptrint = ptrtoint ptr %a to i64 diff --git a/tests/alive-tv/memory/nullptr-align.srctgt.ll b/tests/alive-tv/memory/nullptr-align.srctgt.ll index a87ab3593..e130df8fa 100644 --- a/tests/alive-tv/memory/nullptr-align.srctgt.ll +++ b/tests/alive-tv/memory/nullptr-align.srctgt.ll @@ -1,3 +1,5 @@ +; ERROR: Source is more defined than target + define i8 @src() null_pointer_is_valid { %v = load i8, ptr null, align 4 ret i8 %v diff --git a/tools/transform.cpp b/tools/transform.cpp index 821d1022e..59d585710 100644 --- a/tools/transform.cpp +++ b/tools/transform.cpp @@ -1627,53 +1627,21 @@ void Transform::preprocess() { if (config::tgt_is_asm) { tgt.getFnAttrs().set(FnAttrs::Asm); - // all memory blocks are considered to have a size multiple of alignment - // since asm memory accesses won't trap as it won't cross the page boundary - vector>> to_add; + // there are no #side-effect things in assembly vector to_remove; for (auto &bb : src.getBBs()) { for (auto &i : bb->instrs()) { - if (auto *load = dynamic_cast(&i)) { - auto align = load->getAlign(); - auto size = load->getMaxAccessSize(); - if (size % align) { - static IntType i64("i64", 64); - auto bytes = make_unique(i64, round_up(size, align)); - to_add.emplace_back(load, make_unique( - vector{&load->getPtr(), bytes.get()}, - Assume::Dereferenceable)); - src.addConstant(std::move(bytes)); - } - } else if (auto *call = dynamic_cast(&i)) { + if (auto *call = dynamic_cast(&i)) { if (call->getFnName() == "#sideeffect") { to_remove.emplace_back(const_cast(&i)); } } } - for (auto &[i, assume] : to_add) { - bb->addInstrAt(std::move(assume), i, false); - } for (auto &i : to_remove) { bb->delInstr(i); } - to_add.clear(); to_remove.clear(); } - - // increase size of global variables to be a multiple of alignment - for (auto gv : tgt.getGlobalVars()) { - if (gv->isArbitrarySize()) - continue; - auto align = gv->getAlignment(); - auto sz = gv->size(); - auto newsize = round_up(sz, align); - if (newsize != sz) { - gv->increaseSize(newsize); - if (auto src_gv = dynamic_cast( - src.getGlobalVar(gv->getName()))) - src_gv->increaseSize(newsize); - } - } } remove_unreachable_bbs(src);