Skip to content

Conversation

@anoopkg6
Copy link
Contributor

Fix Endianess issue with getting shadow 4 bytes corresponding to the first origin pointer.

@llvmbot
Copy link
Member

llvmbot commented Oct 10, 2025

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: None (anoopkg6)

Changes

Fix Endianess issue with getting shadow 4 bytes corresponding to the first origin pointer.


Full diff: https://github.com/llvm/llvm-project/pull/162881.diff

1 Files Affected:

  • (modified) llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp (+8-2)
diff --git a/llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
index 5ba2167859490..b4f88779b00c0 100644
--- a/llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
@@ -2187,8 +2187,14 @@ std::pair<Value *, Value *> DFSanFunction::loadShadowFast(
       // and then the entire shadow for the second origin pointer (which will be
       // chosen by combineOrigins() iff the least-significant half of the wide
       // shadow was empty but the other half was not).
-      Value *WideShadowLo = IRB.CreateShl(
-          WideShadow, ConstantInt::get(WideShadowTy, WideShadowBitWidth / 2));
+      Value *WideShadowLo =
+          F->getParent()->getDataLayout().isLittleEndian()
+              ? IRB.CreateShl(
+                    WideShadow,
+                    ConstantInt::get(WideShadowTy, WideShadowBitWidth / 2))
+              : IRB.CreateAnd(
+                    WideShadow,
+                    ConstantInt::get(WideShadowTy, 0xFFFFFFFF00000000ULL));
       Shadows.push_back(WideShadow);
       Origins.push_back(DFS.loadNextOrigin(Pos, OriginAlign, &OriginAddr));
 

@dtcxzyw dtcxzyw requested review from fmayer and vitalybuka October 10, 2025 16:40
@thurstond
Copy link
Contributor

Please add "[dfsan]" to the title

ConstantInt::get(WideShadowTy, WideShadowBitWidth / 2))
: IRB.CreateAnd(
WideShadow,
ConstantInt::get(WideShadowTy, 0xFFFFFFFF00000000ULL));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be rewritten in terms of WideShadowBitWidth?

(I think it's something like (1 - (1 << (WideShadowBitWidth / 2))) << (WideShadowBitWidth / 2))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right way of writing this expression. Thanks.

@thurstond thurstond requested a review from browneee October 10, 2025 17:13
@vitalybuka
Copy link
Collaborator

I don't believe we support other that little endian. So there is no point to complicate code for that.

@thurstond
Copy link
Contributor

I don't believe we support other that little endian. So there is no point to complicate code for that.

They are adding support for SystemZ (#162195).

@thurstond
Copy link
Contributor

Also, RISC-V has an exciting big-endian extension (/cc @fmayer)

@vitalybuka
Copy link
Collaborator

I don't believe we support other that little endian. So there is no point to complicate code for that.

They are adding support for SystemZ (#162195).

Is #162195 enough?
I'd expect sanitizers does a lot of assumptions on little-endianness.

@anoopkg6
Copy link
Contributor Author

anoopkg6 commented Oct 13, 2025 via email

Copy link
Collaborator

@vitalybuka vitalybuka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a test?

@anoopkg6 anoopkg6 changed the title Fix Endianess issue [dfsan] Fix Endianess issue Oct 29, 2025
@anoopkg6
Copy link
Contributor Author

Added origin_endianness.c. This is a reduced test from existing test origin_ldst.c in dfsan testsuite. There are several tests in origin_ldst.c for combinations of partial load/store. Reduced test in origin_endianness.c is the only test case that
fails on SystemZ. Added new test origin_endianness.c does not test anything new than existing dfsan tests. It only highlights the issue of endianness.

@thurstond
Copy link
Contributor

Reduced test in origin_endianness.c is the only test case that
fails on SystemZ

Does the test case need an XFAIL for SystemZ? Otherwise it will break SystemZ buildbots/anyone running tests on SystemZ.

@anoopkg6
Copy link
Contributor Author

I think XFAIL is not needed for this test. This is a subset of already existing test origin_ldst.c in dfsan tests. Moreover, DFSan has not yet been enabled for SystemZ.

@thurstond
Copy link
Contributor

I think XFAIL is not needed for this test. This is a subset of already existing test origin_ldst.c in dfsan tests. Moreover, DFSan has not yet been enabled for SystemZ.

Oh right, thanks!

@anoopkg6
Copy link
Contributor Author

anoopkg6 commented Nov 6, 2025

@uweigand

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants