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

Define relocations for ULEB128 value: R_RISCV_SET_ULEB128 and R_RISCV_SUB_ULEB128 #361

Merged
merged 1 commit into from
Mar 30, 2023

Conversation

kito-cheng
Copy link
Collaborator

@kito-cheng kito-cheng commented Jan 30, 2023

ULEB128 has used in DWARF and exception handling table, it used to record value, or record the distance between two symbols.

The later one would be an issue for debug info and exception handling table, since the symbol address might updated during, then the value might incorrect after relaxation, some of those field has alternative encoding type, but some new field defined in DWARF 5 ins't provide alternative format other than ULEB128, e.g. DW_RLE_offset_pair, DW_RLE_startx_length, DW_RLE_startx_endx and DW_RLE_start_length.

This PR basically same as #162, but updated to the trunk, and also document ULEB128 should not shrink the size of the data, since we never know the data used in where and does the length has recorded in somewhere or not, so this would be most safe way.

binutils patch: https://sourceware.org/pipermail/binutils/2020-January/109672.html
LLVM patch: https://reviews.llvm.org/D142879
LLD patch: https://reviews.llvm.org/D142880

@kito-cheng kito-cheng requested review from MaskRay and jrtc27 January 30, 2023 10:31
@kito-cheng
Copy link
Collaborator Author

Update the link of LLVM + LLD patch.

riscv-elf.adoc Show resolved Hide resolved
@MaskRay
Copy link
Collaborator

MaskRay commented Jan 31, 2023

Deifne relocation for ULEB128 value

Define. Suggest: just specify the full names of the two new relocation types.

@kito-cheng kito-cheng changed the title Deifne relocation for ULEB128 value Define relocations for ULEB128 value: R_RISCV_SET_ULEB128 and R_RISCV_SUB_ULEB128 Jan 31, 2023
@kito-cheng
Copy link
Collaborator Author

Update:

  • Update title:
  • Add note to mention assembler should preserve space for ULEB128 data
  • Mention linker should report error if data size is extended

@kito-cheng
Copy link
Collaborator Author

Changes:

  • Rebased
  • Change number to 60, 61 due to R_RISCV_PLT32 has added.

@kito-cheng kito-cheng requested a review from Nelson1225 March 27, 2023 09:36
@Nelson1225
Copy link
Collaborator

LGTM, it have been expected to support this feature for a long time :)

@kito-cheng
Copy link
Collaborator Author

@rui314 @MaskRay do you mind review it again if it's look good to you? :)

Copy link
Collaborator

@rui314 rui314 left a comment

Choose a reason for hiding this comment

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

Overall looking good.

riscv-cc.adoc Outdated Show resolved Hide resolved
riscv-elf.adoc Outdated Show resolved Hide resolved
riscv-elf.adoc Outdated Show resolved Hide resolved
…_SUB_ULEB128

ULEB128 has used in DWARF and exception handling table, it used to
record value, or record the distance between two symbols.

The later one would be an issue for debug info and exception handling
table, since the symbol address might updated during, then the value
might incorrect after relaxation, some of those field has alternative
encoding type, but some new field defined in DWARF 5 ins't provide
alternative format other than ULEB128, e.g. DW_RLE_offset_pair,
DW_RLE_startx_length, DW_RLE_startx_endx and DW_RLE_start_length.

This PR basically same as #162, but updated to the trunk,
and also document ULEB128 should not shrink the size of the data,
since we never know the data used in where and does the length has
recorded in somewhere or not, so this would be most safe way.

binutils patch: https://sourceware.org/pipermail/binutils/2020-January/109672.html
@kito-cheng
Copy link
Collaborator Author

Thanks everyone! gonna merge this!

@kito-cheng kito-cheng merged commit 4b4fb6a into master Mar 30, 2023
@kito-cheng kito-cheng deleted the uleb128-reloc branch March 30, 2023 06:00
.2+| 60 .2+| SET_ULEB128 .2+| Static | _ULEB128_ .2+| Local label assignment <<uleb128-note,*note>>
<| S + A
.2+| 61 .2+| SUB_ULEB128 .2+| Static | _ULEB128_ .2+| Local label subtraction <<uleb128-note,*note>>
<| V - S + A
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I think this needs to be V - S - A for consistency with other SUB relocations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch! it actually should be V - (S + A) but V -S - A would be consistency with others.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hot-fix applied: 4f7de69

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.

4 participants