[SandboxVectorizer][NFCI] Fix use of possibly-uninitialized scalar. #122201

tylanphear · 2025-01-09T01:17:34Z

The EraseCallbackID field is not always initialized in the ctor for SeedCollector; if not, it will be used uninitialized by its dtor. This could potentially lead to the erasure of a random callback, leading to a bug.

Fixed by making CallbackID an opaque type, which is always default-initialized to an invalid ID.

The `EraseCallbackID` field is not always initialized in the ctor for SeedCollector, if not, it will be used uninitialized by its dtor. This could potentially lead to the erasure of a random callback, leading to a bug. Fixed by initializing to a purposefully invalid ID (`std::numeric_limits<ContextID>::max()`)

llvmbot · 2025-01-09T01:18:08Z

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-vectorizers

Author: Tyler Lanphear (tylanphear)

Changes

The EraseCallbackID field is not always initialized in the ctor for SeedCollector; if not, it will be used uninitialized by its dtor. This could potentially lead to the erasure of a random callback, leading to a bug.

Fixed by initializing to a purposefully invalid ID (std::numeric_limits<ContextID>::max())

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

1 Files Affected:

(modified) llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/SeedCollector.h (+3-1)

diff --git a/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/SeedCollector.h b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/SeedCollector.h
index 6e16a84d832e5e..cc4ebb5c508414 100644
--- a/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/SeedCollector.h
+++ b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/SeedCollector.h
@@ -292,7 +292,9 @@ class SeedCollector {
   SeedContainer StoreSeeds;
   SeedContainer LoadSeeds;
   Context &Ctx;
-  Context::CallbackID EraseCallbackID;
+  Context::CallbackID EraseCallbackID =
+      std::numeric_limits<Context::CallbackID>::max();
+
   /// \Returns the number of SeedBundle groups for all seed types.
   /// This is to be used for limiting compilation time.
   unsigned totalNumSeedGroups() const {

vporpo · 2025-01-09T19:20:27Z

@Sterling-Augustine shouldn't we always register the callback in the constructor, before the early return?

@tylanphear Setting it to max() may not be the correct fix, because Ctx.unregisterEraseInstrCallback() in the SeedCollector's constructor may not work well with it.

slackito · 2025-01-09T20:09:58Z

Nice find! I think the right place to fix this is in the sandboxir::Context class, so that client code doesn't have to worry about initializing the callback ID to an invalid value. Right now a CallbackID is just an integer, but it's used as an opaque ID so we could have something like:

class CallbackID {
  uint64_t id = std::numeric_limits<uint64_t>::max();
};

and this way unregistering an uninitialized ID from anywhere would always trigger an assertion failure (in an asserts-enabled build), or at least not collide with any registered IDs.

What do you think?

tylanphear · 2025-01-09T20:17:18Z

It seems like unconditionally setting the callback ID is a reasonable solution. Using an opaque ID (with a default initializer set to an invalid ID) also seems reasonable, though perhaps a broader change than I'd be comfortable committing without further approvals.

vporpo · 2025-01-09T20:27:14Z

The fact that CallbackID is a uint64_t is an implementation detail, so we should discourage having the user initialize it with an integer value. Also the user should not have to worry about initializing it at all. So I am in favor of the opaque ID fix.

…n Context

tylanphear · 2025-01-09T23:03:43Z

@slackito, @vporpo, thank you both for your prompt review. I've modified the patch according to your suggestions to use an opaque ID type. Please let me know how you think the patch looks in its current state.

vporpo

Thanks for the patch! It looks good in general, just a few minor comments.

vporpo · 2025-01-09T23:11:28Z

llvm/include/llvm/SandboxIR/Context.h

+  public:
+    // Uses a 64-bit integer so we don't have to worry about the unlikely case
+    // of overflowing a 32-bit counter.
+    using ReprTy = uint64_t;


What does "Repr" stand for in ReprTy?

Representation, as uint64_t is the underlying representation. Open to suggestion on the naming here.

Wdyt about ValTy?

Seems reasonable to me.

vporpo · 2025-01-09T23:12:04Z

llvm/include/llvm/SandboxIR/Context.h

+    // Uses a 64-bit integer so we don't have to worry about the unlikely case
+    // of overflowing a 32-bit counter.
+    using ReprTy = uint64_t;
+


nit: I would define the reserved uninitialized value here as a public value:

static constexpr const ReprTy UninitVal = std::numeric_limits<ReprTy>::max();

Done, called it InvalidVal.

vporpo · 2025-01-09T23:14:03Z

llvm/lib/SandboxIR/Context.cpp

@@ -686,7 +686,7 @@ void Context::runMoveInstrCallbacks(Instruction *I, const BBIterator &WhereIt) {
 Context::CallbackID Context::registerEraseInstrCallback(EraseInstrCallback CB) {
  assert(EraseInstrCallbacks.size() <= MaxRegisteredCallbacks &&
         "EraseInstrCallbacks size limit exceeded");
-  CallbackID ID = NextCallbackID++;
+  CallbackID ID{NextCallbackID++};


We should probably have a check here that NextCallbackID is not equal to the reserved value, with something like:

assert(NextCallbackID != CallbackID::UninitVal && "Matches the reserved value!");

Hmm it's probably better to put this check in the constructor.

Makes sense; put an assert in the constructor.

llvm/include/llvm/SandboxIR/Context.h

vporpo

Thanks for the changes! LGTM. @slackito any other comments?

slackito

Looks good, thank you!

…lvm#122201) The `EraseCallbackID` field is not always initialized in the ctor for SeedCollector; if not, it will be used uninitialized by its dtor. This could potentially lead to the erasure of a random callback, leading to a bug. Fixed by making `CallbackID` an opaque type, which is always default-initialized to an invalid ID.

tylanphear requested a review from vporpo January 9, 2025 01:17

llvmbot added vectorizers llvm:transforms labels Jan 9, 2025

vporpo requested a review from Sterling-Augustine January 9, 2025 19:17

vporpo requested a review from slackito January 9, 2025 19:20

Make CallbackID an opaque type, and hide its implementation details i…

9344694

…n Context

llvmbot added the llvm:SandboxIR label Jan 9, 2025

tylanphear added 2 commits January 9, 2025 15:00

Fix accidental explicit usage of uint64_t

3c8349c

Remove unnecessary diff noise.

11901e0

vporpo reviewed Jan 9, 2025

View reviewed changes

slackito reviewed Jan 9, 2025

View reviewed changes

llvm/include/llvm/SandboxIR/Context.h Outdated Show resolved Hide resolved

Address review comments.

803fc9c

vporpo approved these changes Jan 10, 2025

View reviewed changes

slackito approved these changes Jan 10, 2025

View reviewed changes

tylanphear merged commit 4c0a0f7 into llvm:main Jan 10, 2025
8 checks passed

tylanphear deleted the fix_uninitialized_scalar branch January 10, 2025 19:06

[SandboxVectorizer][NFCI] Fix use of possibly-uninitialized scalar. #122201

[SandboxVectorizer][NFCI] Fix use of possibly-uninitialized scalar. #122201

Uh oh!

Conversation

tylanphear commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vporpo commented Jan 9, 2025

Uh oh!

slackito commented Jan 9, 2025

Uh oh!

tylanphear commented Jan 9, 2025

Uh oh!

vporpo commented Jan 9, 2025

Uh oh!

tylanphear commented Jan 9, 2025

Uh oh!

vporpo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vporpo left a comment

Choose a reason for hiding this comment

Uh oh!

slackito left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tylanphear commented Jan 9, 2025 •

edited

Loading

llvmbot commented Jan 9, 2025 •

edited

Loading