Skip to content

chore(string): Add memcchr function - #3430

Open
stephanmeesters wants to merge 1 commit into
TheSuperHackers:mainfrom
stephanmeesters:chore/memcchr
Open

stephanmeesters wants to merge 1 commit into
TheSuperHackers:mainfrom
stephanmeesters:chore/memcchr

Conversation

@stephanmeesters

@stephanmeesters stephanmeesters commented Oct 5, 2026 •

Copy link
Copy Markdown

This adds the memcchr function, which finds the first byte that differs from a given value.

Provided are three versions: SSE2 (for win32 preset), VC6 inline assembly (also uses SSE2), and a portable version.

AI was used. Checked for correctness between versions and reference versions.

@coderabbitai

This comment was marked as spam.

@stephanmeesters stephanmeesters added Minor Severity: Minor < Major < Critical < Blocker System Is Systems related labels Oct 5, 2026
@greptile-apps

greptile-apps Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

[Medium risk] Adds a new string search utility function.

The PR should not merge until the 32-bit search works on CPUs without SSE2 and the repository style requirements are met.

Findings

  1. P1 Older CPUs crash on search ▶
  2. P2 Standalone header fails to build ▶
  3. P2 Search versions lack tests ▶
  4. P2 Actions share condition lines ▶
  5. P2 Null return breaks style rule ▶
Summary

This PR adds memcchr, with portable, SSE2, and VC6 assembly versions.

  • The 32-bit SSE2 selection needs a CPU check before callers can safely use it on older hardware.
  • The portable header needs its own type include, and the three versions need shared tests.
  • Two new lines break repository style requirements.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A["Include stringex.h"] --> B{"VC6?"}
  B -- Yes --> C["VC6 assembly memcchr"]
  B -- No --> D{"x86 or SSE2 build?"}
  D -- Yes --> E["SSE2 memcchr"]
  D -- No --> F["Portable memcchr"]
Loading

Reviews (1) · Last reviewed commit: "chore(string): Add memcchr function"

Comment on lines +70 to +74
#if defined(_MSC_VER) && _MSC_VER < 1300
#include "stringex_memcchr_x86asm.inl"
#else
#if defined(_M_IX86) || defined(_M_X64) || defined(__SSE2__)
#include "stringex_memcchr_sse2.inl"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Older CPUs crash on search

If a 32-bit CPU lacks SSE2, calling memcchr runs instructions it cannot use and crashes. The VC6 build selects the assembly version without checking the CPU. The 32-bit MSVC path has the same problem. Select the portable version when SSE2 is unavailable.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Select on runtime?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes—runtime dispatch is the appropriate fix for 32-bit builds that may run on pre-SSE2 CPUs.

Keep the SSE2 implementation compiled for x86, but expose it under a separate internal name and have memcchr select between it and the portable implementation based on CPUID leaf 1, EDX bit 26. Cache the selected implementation so CPUID is only queried once. The VC6 path should perform the same check before calling the inline-assembly implementation; it must not execute any SSE2 instruction during feature detection.

x64 can continue using SSE2 unconditionally, since SSE2 is mandatory there.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Chat Gippy says

If the executable is intended to run on any x86-64 CPU

No runtime guard is normally needed. SSE2 is part of the mandatory x86-64 baseline. Every conforming x86-64 CPU supports SSE2.

For example:

#include <emmintrin.h>

__m128i add(__m128i a, __m128i b) {
return _mm_add_epi32(a, b);
}

Compiling this as a normal x86-64 executable is safe:

gcc -O2 -march=x86-64 foo.c -o foo

The resulting program can assume SSE2 when running on an x86-64 processor.

If you also want a 32-bit x86 executable

Then yes, you need to consider the CPU baseline. SSE2 is not guaranteed on all 32-bit x86 processors.

You have two common approaches:

1. Require SSE2

Compile with SSE2 enabled and document that requirement:

gcc -m32 -msse2 -O2 foo.c -o foo

Then running it on a CPU without SSE2 is not safe—the process may fault when it executes an SSE2 instruction.

2. Runtime dispatch

If you need to support CPUs both with and without SSE2, keep a non-SSE2 implementation and select it at runtime:

if (cpu_has_sse2()) {
sse2_version();
} else {
scalar_version();
}

The important part is that the compiler must not emit SSE2 instructions into the CPU-independent path. Typically you compile the SSE2 implementation separately with the appropriate target options and dispatch to it only after detection.

One important distinction

There are two different things that can happen:

gcc -msse2 ...

means “SSE2 instructions are allowed/required in the generated code.”

Whereas:

gcc -march=x86-64-v3 ...

means “the target CPU is assumed to support a larger collection of ISA extensions.”

For x86-64, -msse2 by itself doesn't imply AVX, AVX2, etc. SSE2 is already part of the x86-64 baseline.

So, in short:

Target SSE2 runtime guard?
x86-64 No — SSE2 is guaranteed
32-bit x86, SSE2 required No, but unsupported CPUs cannot run it
32-bit x86, broad CPU support Yes, use runtime dispatch
ARM/other architecture SSE2 code isn't executable there; provide another implementation

If you're asking because you're compiling with GCC/Clang and using -msse2, -march, or __attribute__((target("sse2"))), I can also explain the subtle cases where the compiler can accidentally put SSE2 instructions into the supposedly generic code path.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Chat Gippy is right about the x64 case: _M_X64 can select the SSE2 implementation unconditionally because SSE2 is part of the x86-64 baseline. A runtime guard is not needed there.

The concern is specifically the 32-bit paths. _M_IX86 does not mean that the target CPU supports SSE2, so the current condition selects stringex_memcchr_sse2.inl for every 32-bit MSVC build. The VC6 branch is even more direct: it always includes the SSE2 inline-assembly implementation. Those binaries can still run on pre-SSE2 32-bit CPUs, where the first SSE2 instruction will fault.

Thus the fix should be scoped to 32-bit builds: retain unconditional SSE2 for _M_X64, but either document an SSE2 requirement for 32-bit binaries or add runtime dispatch to the portable implementation for 32-bit targets. Builds that explicitly define __SSE2__ already express that requirement and do not need an additional guard.

Comment thread Dependencies/Utility/Utility/stringex_memcchr.inl
Comment on lines +70 to +78
#if defined(_MSC_VER) && _MSC_VER < 1300
#include "stringex_memcchr_x86asm.inl"
#else
#if defined(_M_IX86) || defined(_M_X64) || defined(__SSE2__)
#include "stringex_memcchr_sse2.inl"
#else
#include "stringex_memcchr.inl"
#endif
#endif

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Search versions lack tests

memcchr has three separate versions, including hand-encoded assembly, but this change adds no tests for their results or length boundaries. Shared tests for the first mismatch, all-equal input, and lengths around each block size would catch differences between builds.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

tests can be added after #3405 is merged

while (n >= 8) {
uint64_t v;
memcpy(&v, p, sizeof(v));
if (v != repeated64) break;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Actions share condition lines

The new if puts break on the condition's line. The repository requires statement bodies on separate lines so developers can set a breakpoint on the action. The SSE2 version also uses this pattern. Put these bodies on their own lines before merging.

Rule Used: Always place if/else/for/while statement bodies on separate lines from the condition to allow precise debugger breakpoint placement. Prefer the format: cpp if (condition) doSomethingNextLine(); over: ```cpp if (condition) doSomethingOnSam... (source)

Learned From
TheSuperHackers/GeneralsGameCode#2067

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

++p;
--n;
}
return 0;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Null return breaks style rule

The portable version returns 0 as a null pointer. The repository requires nullptr for null pointer literals in C++, as the SSE2 version already uses. Change this return before merging.

Suggested change
return 0;
return nullptr;

Rule Used: Use nullptr instead of NULL for null pointer literals in C++ code (source)

Learned From
TheSuperHackers/GeneralsGameCode#2067

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

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

Labels

Minor Severity: Minor < Major < Critical < Blocker System Is Systems related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants