Add more description for .option directive#68
Conversation
jim-wilson
left a comment
jim-wilson
left a comment
There was a problem hiding this comment.
Some linkers don't support relaxation, but that probably doesn't need to be explained here. if it does, then maybe something like "if the linker supports it" added to the relax explanation would be good enough.
|
Changes:
|
|
@jrtc27 does this change look good to you after apply your revision? |
|
@luismarques @asb @jrtc27 did you mind did a final round review? |
| Enable/disable linker relaxation for the following code region if the linker | ||
| supports it. |
There was a problem hiding this comment.
"if the linker supports it" -> I think this enables it unconditionally, no? At least Clang will just enable it, and if the linker doesn't support it (LLD...) then it craps out.
I don't know if it's worth mentioning that this also includes emission ofR_RISCV_ALIGN relocations?
| .endm | | end macro definition | ||
| .type | symbol, @function | accepted for source compatibility | ||
| .option | {rvc,norvc,pic,nopic,push,pop} | RISC-V options | ||
| .option | {rvc,norvc,pic,nopic,relax,norelax,push,pop} | RISC-V options, more detailed description in [.option](#.option). |
There was a problem hiding this comment.
Reformat table, to realign the |s?
Nit: "RISC-V options. Refer to .option for a more detailed description."
There was a problem hiding this comment.
I would prefer don't realign this time since we might need to realign for .option arch, soon, my thought is we can did that when converting the format to AsciiDoc.
|
|
||
| #### `rvc`/`norvc` | ||
|
|
||
| Enable/disable C-extension for the following code region. |
There was a problem hiding this comment.
"C-extension" -> "The C extension"
I don't know if the word "region" in "the following code region" kind of implies that this has limited scope, like an #if ... #endif region. I'll let the native speakers nitpick that :)
There was a problem hiding this comment.
@aswaterman did you having some suggestion on the wording about region?
I hadn't noticed your earlier comment, Jim. I think that phrasing ends up being misleading. When using this option the compiler will emit relaxation relocations without checking for linker support, and then the linker will complain if it doesn't actually support such relaxations. Maybe just state that the linker needs to support relaxations? |
kito-cheng
force-pushed
the
doc-option
branch
2 times, most recently
from
fdd4fcf to
27515d2
Compare
|
Changes:
|
|
Changes:
|
No additional comments from me. |
|
I don't think I have permission in this repo/org to approve anything, but the technical issues I pointed out seem to have been addressed, so it LGTM. |
|
@luismarques @asb thanks you guys, I gonna merge this :) |
5 participants
No description provided.