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

Re-land: [FIRRTL][CAPI] Add more functions for discriminating and querying type #7972

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

SpriteOvO
Copy link
Member

@SpriteOvO SpriteOvO commented Dec 12, 2024

Re-push #7960, closes #7970.

In #7960, I used memcmp for comparing 2 struct FIRRTLBundleField, this is problematic and may return unexpected results since 2 MlirXXX different opaque pointer structs may be the same content.

bool bundleFieldEqual(const FIRRTLBundleField *lhs,
                      const FIRRTLBundleField *rhs) {
  return mlirIdentifierEqual(lhs->name, rhs->name) &&
         lhs->isFlip == rhs->isFlip && mlirTypeEqual(lhs->type, rhs->type);
}

After replacing it with function bundleFieldEqual above, it should be correct and the crash no longer occurs on Windows, and Valgrind no longer reports Conditional jump or move depends on uninitialised value(s). The uninitialized memory error from Valgrind could be because memcmp accessed struct padding, but I'm confused as to why Windows reports it as 0xc0000409 (STATUS_STACK_BUFFER_OVERRUN). 😶‍🌫️

Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

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

SGTM

@SpriteOvO SpriteOvO merged commit 7a98d67 into llvm:main Dec 13, 2024
4 checks passed
@SpriteOvO SpriteOvO deleted the firrtl-more-c-api branch December 13, 2024 01:16
@SpriteOvO SpriteOvO changed the title [FIRRTL][CAPI] Add more functions for discriminating and querying type Re-land: [FIRRTL][CAPI] Add more functions for discriminating and querying type Dec 13, 2024
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.

[CAPI] Windows and Nightly Failure after #7960
2 participants