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 LdScript emission #708

Merged
merged 1 commit into from
Aug 27, 2024
Merged

Fix LdScript emission #708

merged 1 commit into from
Aug 27, 2024

Conversation

makslevental
Copy link
Collaborator

@makslevental makslevental commented Aug 27, 2024

Without explicit locations for these sections, they get placed in arbitrary locations. The introduced linker flag will error out if new sections appear (and don't have specific locations/assignments).

See Xilinx/mlir-aie#1717

Copy link
Collaborator

@jtuyls jtuyls left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +70 to +93
.comment : {
*(.comment*)
}
.symtab : {
*(.symtab)
}
.shstrtab : {
*(.shstrtab)
}
.strtab : {
*(.strtab)
}
.tctmemtab : {
*(.tctmemtab)
}
.rtstab : {
*(.rtstab)
}
.eoltab : {
*(.eoltab)
}
.chesstypeannotationtab : {
*(.chesstypeannotationtab)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Orthogonal to this PR specifically, but are all of these needed? I.e. aren't some of these metadata and wouldn't we be able to make the elf smaller by excluding those?

Copy link
Collaborator Author

@makslevental makslevental Aug 27, 2024

Choose a reason for hiding this comment

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

The way this works is with these sections specified explicitly, if any of the ELFs contain these sections, then those sections will be placed at these locations. E.g., this PR failed at first because I turned on =error and mm.o had a .chesstypeannotationtab section that currently gets placed at an arbitrary location.

So the sections are already there and are already getting placed into the ELF. -Wl,--gc-sections pertains to your question and there I do not know what counts as "unused" and therefore "will be gced". But what I think is intuitively true, if the sections are empty they will be gced.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, thanks for the explanation! I am a rookie on ld scripts so will take your word for it :) At some point, it might be interesting to see what sections are actually copied into the elf.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I ain't no pro either - 90% of what I'm saying is just stuff I'm absorbing by osmosis by reading PRs/discussions in llvm-aie.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually that's not true - I did actually read some "docs" on it (shocking I know); I read about half of this thing https://mcyoung.xyz/2021/06/01/linker-script/.

@makslevental makslevental enabled auto-merge (squash) August 27, 2024 20:56
@makslevental makslevental merged commit c2b9f58 into main Aug 27, 2024
6 checks passed
@makslevental makslevental deleted the makslevental/fix-ld-script-v2 branch August 27, 2024 21:10
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