Skip to content

fix: correct memcpy size in or_block function #24567

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

gchumillas
Copy link
Contributor

@gchumillas gchumillas commented May 24, 2025

Fixes #24529

Caution

This solution was provided by an AI after several hours of research, as I'm not familiar with the base code. Review it carefully and reject it if necessary.

All tests passed:

v self
v test-self

Additional context (conversacion with Claude Sonnet 4):

gchumillas: Hello, could you explain to me why these changes were made?

@@ -7085,10 +7090,17 @@ fn (mut g Gen) or_block(var_name string, or_block ast.OrExpr, return_type ast.Ty
 			if g.fn_decl == unsafe { nil } || g.fn_decl.return_type == ast.void_type {
 				g.writeln('\treturn;')
 			} else {
-				styp := g.styp(g.fn_decl.return_type)
-				err_obj := g.new_tmp_var()
-				g.writeln2('\t${styp} ${err_obj};', '\tmemcpy(&${err_obj}, &${cvar_name}, sizeof(${result_name}));')
-				g.writeln('\treturn ${err_obj};')
+			   styp := g.styp(g.fn_decl.return_type)
+			   err_obj := g.new_tmp_var()
+			   g.writeln("\t${styp} ${err_obj} = {0};")
+			   if g.fn_decl.return_type.has_flag(.result) {
+				   g.writeln("\t${err_obj}.is_error = true;");
+			   } else if g.fn_decl.return_type.has_flag(.option) {
+				   g.writeln("\t${err_obj}.state = 2;");
+			   }
+			   g.writeln("\t${err_obj}.err = ${cvar_name}${tmp_op}err;");
+			   // Optionally zero data: memset(&${err_obj}.data, 0, sizeof(${styp}) - offsetof(${styp}, data));
+			   g.writeln("\treturn ${err_obj};")
 			}
 		}
 	} else if or_block.kind == .propagate_option {

In particular, I'm concerned that the following line was removed:

g.writeln2('\t${styp} ${err_obj};', '\tmemcpy(&${err_obj}, &${cvar_name}, sizeof(${result_name}));')

Excellent question. Let me explain why these changes were made and why it was necessary to remove that specific line.

Problem Context

The bug we were fixing was that the V compiler generated incorrect C code for error propagation with the ! operator. Specifically, the generated code tried to access the .is_error field on option type structures, but option structures don't have that field - they only have .state.

Problem with the removed line

The line you're concerned about:

g.writeln2('\t${styp} ${err_obj};', '\tmemcpy(&${err_obj}, &${cvar_name}, sizeof(${result_name}));')

Had several problems:

  1. Incorrect use of sizeof(${result_name}): This line always used sizeof(${result_name}) regardless of whether the type was result or option. This caused problems because:
    • result and option types have different structures
    • result_name is a constant that refers specifically to result types
    • Copying memory using the wrong size could corrupt data
  2. Blind memory copy: The memcpy function copied the entire structure without considering that:
    • result types use .is_error (boolean)
    • option types use .state (byte with values 0, 1, 2)
    • The fields have different semantics and layouts

The implemented solution

The new code is more explicit and correct:

// Clean initialization
g.writeln("\t${styp} ${err_obj} = {0};")

// Type-specific configuration
if g.fn_decl.return_type.has_flag(.result) {
    g.writeln("\t${err_obj}.is_error = true;");
} else if g.fn_decl.return_type.has_flag(.option) {
    g.writeln("\t${err_obj}.state = 2;");
}

// Only copy the error (common field)
g.writeln("\t${err_obj}.err = ${cvar_name}${tmp_op}err;");

Advantages of the new implementation:

  1. Type safe: Only sets fields that actually exist in each structure
  2. Semantically correct:
    • .is_error = true for results (indicates error)
    • .state = 2 for options (indicates error state)
  3. Explicit initialization: = {0} initializes the entire structure to zero
  4. Only copies what's needed: Only transfers the .err field which is common to both types

Verification that no functionality was lost

The original function tried to copy everything with memcpy, but this was problematic. The new implementation:

  1. ✅ Initializes the structure correctly
  2. ✅ Sets the appropriate state/error field
  3. ✅ Copies the error message
  4. ✅ Data is initialized to zero (safe behavior)

The comment // Optionally zero data: memset(...) indicates that if necessary, explicit data initialization could be added, but the {0} initialization already handles this correctly.

Result

The tests confirm that functionality remains intact while fixing the bug of incorrect C code generation. Removing the memcpy was necessary because it was causing the fundamental problem we were solving.

Copy link

Connected to Huly®: V_0.6-22933

@JalonSolov
Copy link
Contributor

Need to run v fmt -w vlib/v/gen/c/cgen.v to pass the CI.

@JalonSolov
Copy link
Contributor

From CONTRIBUTING.md: Running these commands will ensure files are formatted before they are commited.

cp cmd/tools/git_pre_commit_hook.vsh .git/hooks/pre-commit
chmod 755 .git/hooks/pre-commit

@gchumillas gchumillas force-pushed the array_assertion_fails_on_macbook_pro_m2 branch from 17703fd to 7920c3f Compare May 24, 2025 21:23
@gchumillas
Copy link
Contributor Author

The AI ​​gave us clues on how to solve the problem, but it also included more code than necessary.

Closing this PR.

@gchumillas gchumillas closed this May 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cgen: array assertion fails on MacBook Pro M2
2 participants