Skip to content

Patches #4474

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

Merged
merged 3 commits into from
Feb 18, 2020
Merged

Patches #4474

merged 3 commits into from
Feb 18, 2020

Conversation

LemonBoy
Copy link
Contributor

No description provided.

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

The bug fix needs work. The other commit looks good.

}
}

if (orig_array_ptr_val->special == ConstValSpecialUndef) {
Copy link
Member

Choose a reason for hiding this comment

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

I think a call to ir_resolve_const_val is appropriate here. Maybe like this (changing some lines above as well):

if (orig_array_ptr_val->special != ConstValSpecialRuntime) {
    if ((err = ir_resolve_const_val(ira->codegen, ira->new_irb.exec, value->base.source_node,
                    orig_array_ptr_val, UndefBad)))
    {
        return ira->codegen->invalid_inst_gen;
    }
    if (orig_array_ptr_val->data.x_ptr.special != ConstPtrSpecialHardCodedAddr) {
        orig_array_ptr_val = const_ptr_pointee(ira, ira->codegen, orig_array_ptr_val,
            elem_ptr_instruction->base.base.source_node);
        if (orig_array_ptr_val == nullptr)
            return ira->codegen->invalid_inst_gen; 
        // and then if we need to make sure the new orig_array_ptr_val is also not
        // undefined then another call to ir_resolve_const_val here.
    }
}

@@ -3,6 +3,15 @@ const builtin = @import("builtin");
const Target = @import("std").Target;

pub fn addCases(cases: *tests.CompileErrorContext) void {
cases.addTest("access of undefined pointer to array",
Copy link
Member

Choose a reason for hiding this comment

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

This test case isn't correct - ram_u32 is a global variable, and it might have been reassigned before entry is executed, so it can't be a compile error detecting use of undefined here without some advanced analysis that Zig is not currently capable of. Also the "use of undefined value" as a compile error is deprecated (See #1947), only certain kinds of use of undefined is invalid, and it gives the message, "use of undefined value here causes undefined behavior", which should be emitted via a call to ir_resolve_const_val (or ir_resolve_const) with the UndefAllowed parameter set to UndefBad.

It's a bit tricky - when obtaining the pointer to a global variable, it is considered to be comptime known, however the memory that it points to is runtime known. So if ir_analyze_instruction_elem_ptr is seeing ConstValSpecialUndef then I think the bug has already occurred elsewhere. ir_analyze_instruction_elem_ptr should be seeing ConstValSpecialRuntime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I got sidetracked by the complex logic and forgot that it was a var. The new patch allows the code to compile (no idea how to test that) and I've added a check to make sure the use of undefined is caught at compile-time.

@LemonBoy LemonBoy force-pushed the saukerkraut branch 2 times, most recently from 35619dd to 3d0291a Compare February 16, 2020 19:42
Collect the declarations to resolve first and run resolve_top_level_decl
on them later.

Closes ziglang#4310
@andrewrk andrewrk merged commit ccca4b5 into ziglang:master Feb 18, 2020
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.

2 participants