Skip to content

Add support for unnamed_attr resolving in linker #31

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
Apr 18, 2025
Merged

Conversation

Jezurko
Copy link
Collaborator

@Jezurko Jezurko commented Apr 7, 2025

No description provided.

@Jezurko Jezurko requested a review from xlauko April 7, 2025 15:48
@Jezurko Jezurko self-assigned this Apr 7, 2025
Comment on lines 326 to 333
if (isAppendingLinkage(srcLinkage) && isAppendingLinkage(dstLinkage)) {
if (srcUnnamedAddr != dstUnnamedAddr) {
pair.src->emitError("Appending variables with different unnamed_addr "
"need to be linked: ")
<< pair.dst->getLoc();
return failure();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Where did this come from?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In llvm-link it happens in lib/Lnker/IRMover.cpp:877. I added it to satisfy the imported test. Should I move it somewhere else?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is to verbose and disctracts from logic.

Add some helper lambda linkError with use here like

return linkError("Appending variables with different unnamed_addr need to be linked:");

Copy link
Member

@xlauko xlauko Apr 17, 2025

Choose a reason for hiding this comment

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

And ideally extract entire thing to separate check function, because there will be more conditions on src and dst here. E.g. result should look like:

 if (isAppendingLinkage(srcLinkage) && isAppendingLinkage(dstLinkage)) {
  if (failed(verifyAppendingLinkageCompatibility(src, dst)))
    return failure();
}

@Jezurko Jezurko force-pushed the robert/unnamedattr branch from 9f339c9 to 9a73cc3 Compare April 10, 2025 13:37
@Jezurko Jezurko force-pushed the robert/unnamedattr branch 2 times, most recently from 6c620f3 to 7021e7f Compare April 18, 2025 09:56
@Jezurko Jezurko force-pushed the robert/unnamedattr branch from 7021e7f to 935fce3 Compare April 18, 2025 10:34
@Jezurko Jezurko merged commit 6d11c66 into main Apr 18, 2025
6 checks passed
@Jezurko Jezurko deleted the robert/unnamedattr branch April 18, 2025 11:38
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