Skip to content
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

Fix another LockViolation case on Windows #14162

Merged
merged 4 commits into from Jan 4, 2023

Conversation

kcbanner
Copy link
Contributor

@kcbanner kcbanner commented Jan 2, 2023

#13864 reverted some changes which were causing breakage if the same c file was compiled twice as part of the same compilation. This PR makes the fixes as suggested by this comment there: #13864 (comment)

Changes:

  • Add an assert that an exclusive lock is help to writeManifest, to catch this error when it happens.
  • Only call writeManifest if an exclusive lock is held.
  • Update tests to assert that a cache hit results in holding a shared lock, instead of writing the manifest

Reproduction steps for the LockViolation:

// build.zig
const std = @import("std");
const Builder = std.build.Builder;

pub fn build(b: *Builder) void {
    const target = b.standardTargetOptions(.{});
    const mode = b.standardReleaseOptions();

    const exe = b.addExecutable("template", null);
    exe.addCSourceFile("main.c", &.{});
    exe.linkLibC();
    exe.setTarget(target);
    exe.setBuildMode(mode);
    exe.install();
}
// main.c
#include <stdio.h>

int main(void)
{
    int x = 12;
    x = (x % 2 == 0) ? x / 2 : 3 * x + 1;

    // Uncomment to cause compilation failure
    //x = (x % 2 == 0) ? x = x / 2 : x = 3 * x + 1;
    return 0;
}
  1. zig build. This compiles normally.
  2. Uncomment the failing line in main.c
  3. zig build. This compile fails due to the C compilation error.
  4. Re-comment the failing line in main.c
  5. zig build. This compiles successfully, but triggers the LockViolation.

Trace:
lock_violation_1

- Add an assert that an exclusive lock is help to writeManifest
- Only call writeManifest in updateCObject if an exclusive lock is held
@kcbanner kcbanner changed the title Fix LockViolation another LockViolation case on Windows Fix another LockViolation case on Windows Jan 2, 2023
@kcbanner kcbanner marked this pull request as draft January 3, 2023 07:57
@matu3ba
Copy link
Contributor

matu3ba commented Jan 4, 2023

Just curious: Is there a way to stress test this anyhow somewhat reliably?

@kcbanner kcbanner marked this pull request as ready for review January 4, 2023 19:26
@andrewrk
Copy link
Member

andrewrk commented Jan 4, 2023

This fix looks just right 👌

@andrewrk andrewrk merged commit a3e2ee0 into ziglang:master Jan 4, 2023
@kcbanner kcbanner deleted the fix_lock_violation_mk2 branch January 8, 2023 22:28
andrewrk pushed a commit that referenced this pull request Jan 9, 2023
- Add an assert that an exclusive lock is help to writeManifest
- Only call writeManifest in updateCObject if an exclusive lock is held
- cache: fixup test to verify hits don't take an exclusive lock, instead of writing the manifest
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.

None yet

3 participants