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.

Sign up for GitHub

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

Jump to bottom

Document the behaviour difference between LLD and GNU LD while using --defsym for library selection #463

Merged
merged 3 commits into from
Aug 15, 2024
Merged

Document the behaviour difference between LLD and GNU LD while using --defsym for library selection #463

merged 3 commits into from
Aug 15, 2024

Conversation

simpal01
Copy link
Collaborator

No description provided.

voltur01
voltur01 reviewed Aug 14, 2024
View reviewed changes
Copy link
Contributor

@voltur01 voltur01 left a comment

Choose a reason for hiding this comment

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

Thank you for adding this! Please see some style and reference suggestions.

docs/migrating.md Outdated
@@ -100,6 +100,9 @@ however there are some known differences to take into account, see:
in the LLD documentation.
* The [LLD and GNU linker incompatibilities](https://maskray.me/blog/2020-12-19-lld-and-gnu-linker-incompatibilities)
blog post.
* When selecting the printf/scanf library variant with `--defsym from=to`, lld includes not only the specified
Copy link
Contributor

Choose a reason for hiding this comment

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

capitalize LLD as above.

Copy link
Contributor

Choose a reason for hiding this comment

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

"library variant" - implementation variant?
and maybe further add implementation variant from picolibc

docs/migrating.md Outdated
@@ -100,6 +100,9 @@ however there are some known differences to take into account, see:
in the LLD documentation.
* The [LLD and GNU linker incompatibilities](https://maskray.me/blog/2020-12-19-lld-and-gnu-linker-incompatibilities)
blog post.
* When selecting the printf/scanf library variant with `--defsym from=to`, lld includes not only the specified
Copy link
Contributor

Choose a reason for hiding this comment

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

add a reference to https://github.com/picolibc/picolibc/blob/main/doc/printf.md after the defsym to be clear when/how it is used.

docs/migrating.md Outdated
@@ -100,6 +100,9 @@ however there are some known differences to take into account, see:
in the LLD documentation.
* The [LLD and GNU linker incompatibilities](https://maskray.me/blog/2020-12-19-lld-and-gnu-linker-incompatibilities)
blog post.
* When selecting the printf/scanf library variant with `--defsym from=to`, lld includes not only the specified
printf/scanf library variant through defsym but also the PICOLIBC_DOUBLE_PRINTF_SCANF variant. To avoid linking
Copy link
Contributor

Choose a reason for hiding this comment

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

do you want to add code formatting for defsym? or ever refer to it as --defsym?

docs/migrating.md Outdated
@@ -100,6 +100,9 @@ however there are some known differences to take into account, see:
in the LLD documentation.
* The [LLD and GNU linker incompatibilities](https://maskray.me/blog/2020-12-19-lld-and-gnu-linker-incompatibilities)
blog post.
* When selecting the printf/scanf library variant with `--defsym from=to`, lld includes not only the specified
printf/scanf library variant through defsym but also the PICOLIBC_DOUBLE_PRINTF_SCANF variant. To avoid linking
them into the application, you must include the --gc-sections linker flag.
Copy link
Contributor

Choose a reason for hiding this comment

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

"them" maybe "the default variant"?

docs/migrating.md Outdated
@@ -100,6 +100,9 @@ however there are some known differences to take into account, see:
in the LLD documentation.
* The [LLD and GNU linker incompatibilities](https://maskray.me/blog/2020-12-19-lld-and-gnu-linker-incompatibilities)
blog post.
* When selecting the printf/scanf library variant with `--defsym from=to`, lld includes not only the specified
printf/scanf library variant through defsym but also the PICOLIBC_DOUBLE_PRINTF_SCANF variant. To avoid linking
them into the application, you must include the --gc-sections linker flag.
Copy link
Contributor

Choose a reason for hiding this comment

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

code format --gc-sections?

@simpal01
Addressing the comments.
83a91c0 
 1. Add backquotes
 2. Add the reference to https://github.com/picolibc/picolibc/blob/main/doc/printf.md
 3. rephrase some words.
voltur01
voltur01 reviewed Aug 14, 2024
View reviewed changes
Copy link
Contributor

@voltur01 voltur01 left a comment

Choose a reason for hiding this comment

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

Thank you for the update! Please see some minor suggestions for your consideration.

docs/migrating.md Outdated
* When selecting the printf/scanf library variant with `--defsym from=to`, lld includes not only the specified
printf/scanf library variant through defsym but also the PICOLIBC_DOUBLE_PRINTF_SCANF variant. To avoid linking
them into the application, you must include the --gc-sections linker flag.
* When selecting the printf/scanf library variant with `--defsym from=to`, see:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* When selecting the printf/scanf library variant with `--defsym from=to`, see:
* When selecting the printf/scanf picolibc implementation variant with `--defsym from=to`, see

docs/migrating.md Outdated
printf/scanf library variant through defsym but also the PICOLIBC_DOUBLE_PRINTF_SCANF variant. To avoid linking
them into the application, you must include the --gc-sections linker flag.
* When selecting the printf/scanf library variant with `--defsym from=to`, see:
[How to use defsym](https://github.com/picolibc/picolibc/blob/main/doc/printf.md), LLD includes not only the specified
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[How to use defsym](https://github.com/picolibc/picolibc/blob/main/doc/printf.md), LLD includes not only the specified
[Printf and Scanf levels in Picolibc](https://github.com/picolibc/picolibc/blob/main/doc/printf.md#printf-and-scanf-levels-in-picolibc), LLD includes not only the

docs/migrating.md Outdated
them into the application, you must include the --gc-sections linker flag.
* When selecting the printf/scanf library variant with `--defsym from=to`, see:
[How to use defsym](https://github.com/picolibc/picolibc/blob/main/doc/printf.md), LLD includes not only the specified
printf/scanf implementation variant from picolibc through defsym but also the PICOLIBC_DOUBLE_PRINTF_SCANF variant.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
printf/scanf implementation variant from picolibc through defsym but also the PICOLIBC_DOUBLE_PRINTF_SCANF variant.
printf/scanf implementation variant from picolibc specified through `--defsym`, but also the default PICOLIBC_DOUBLE_PRINTF_SCANF variant.

@nasherm nasherm self-requested a review August 15, 2024 09:52
nasherm
nasherm approved these changes Aug 15, 2024
View reviewed changes
Copy link
Collaborator

@nasherm nasherm left a comment

Choose a reason for hiding this comment

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

LGTM

@simpal01 simpal01 merged commit 223f9ad into ARM-software:main Aug 15, 2024
1 check passed
Laur59 added a commit to Laur59/LLVM-embedded-toolchain-for-Arm that referenced this pull request Aug 15, 2024
@Laur59
Document the behaviour difference between LLD and GNU LD while using …
5b1a8cc 
…--defsym for library selection (ARM-software#463)

* Document the behaviour difference between LLD and GNU LD while using --defsym for library selection

* Addressing the comments.

 1. Add backquotes
 2. Add the reference to https://github.com/picolibc/picolibc/blob/main/doc/printf.md
 3. rephrase some words.

* Addressing the suggestions about wording/formatting.
Laur59 added a commit to Laur59/LLVM-embedded-toolchain-for-Arm that referenced this pull request Aug 15, 2024
@Laur59
Document the behaviour difference between LLD and GNU LD while using …
737fa74 
…--defsym for library selection (ARM-software#463)

* Document the behaviour difference between LLD and GNU LD while using --defsym for library selection

* Addressing the comments.

 1. Add backquotes
 2. Add the reference to https://github.com/picolibc/picolibc/blob/main/doc/printf.md
 3. rephrase some words.

* Addressing the suggestions about wording/formatting.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@voltur01 voltur01 voltur01 left review comments

@nasherm nasherm nasherm approved these changes

@smithp35 smithp35 Awaiting requested review from smithp35

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@simpal01 @nasherm @voltur01