Skip to content

Conversation

@ZhangHongchen1
Copy link

Hi Josh,
We are preparing to support a new architecture:LoongArch . But we find some common problems of this tool,So I think we can submit the common patches firstly,Then the fllowing LoongArch support.
These three patches solve the following problem:

  1. e_flags is not written in the end file,we should save it,beause it holds processor-specific flags.
  2. clean up the symbol replacement algorithm. When we want add LoongArch support,we find out we should add
    !strcmp(rela->sym->name, ".orc_unwind_ip"). IMO, too many compares in the code is not a good idea, so I clean the code and
    make it easy to read.
  3. when one symbol is not included in the output elf file,the related rela of debug info section should also not be included. If we
    don't follow it,then the rela would refer to an not included symbol which contains random data.

Thanks
Hongchen Zhang

if (rela->type == R_X86_64_32S &&
is_text_section(relasec->base) &&
!is_text_section(sym->sec) &&
rela->type == R_X86_64_32S &&
Copy link
Member

Choose a reason for hiding this comment

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

Please make this a separate commit.

Copy link
Author

Choose a reason for hiding this comment

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

OK

strncmp(rela->sym->name, ".rodata", 7)) {
ERROR("%s+0x%x: can't find replacement symbol for %s+%ld reference",
relasec->base->name, rela->offset, rela->sym->name, rela->addend);
}
Copy link
Member

Choose a reason for hiding this comment

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

If this error is getting removed, there needs to be more justification than "we think it is just ok".

Copy link
Member

Choose a reason for hiding this comment

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

Also, why does .rela.orc_unwind_ip reference .orc_unwind_ip itself? That sounds like a different format compared to the x86 implementation, which only references text addresses.

Copy link
Author

Choose a reason for hiding this comment

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

If this error is getting removed, there needs to be more justification than "we think it is just ok".
OK,Let's leave it not changed.

Copy link
Author

Choose a reason for hiding this comment

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

Also, why does .rela.orc_unwind_ip reference .orc_unwind_ip itself? That sounds like a different format compared to the x86 implementation, which only references text addresses.
Yes,the rela format is a little different.

continue;
list_for_each_entry_safe(rela, saferela, &sec->relas, list)
if (!rela->sym->sec->include)
if (!rela->sym->include || !rela->sym->sec->include) {
Copy link
Member

Choose a reason for hiding this comment

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

Hm, why haven't we seen this problem on x86?

Copy link
Author

@ZhangHongchen1 ZhangHongchen1 Nov 17, 2022

Choose a reason for hiding this comment

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

Yes, I checked the compiled c file fs/aio.c of x86_64.It reference to __dyndbg using the __dyndbg + offset,it did not use the symbols in the __dyndbg section ,so it has no problem.
But LoongArch generate relas using the symbols in __dyndbg(or __verbose in old version kernel) and triggers the problem.
I think we should avoid that condition even if there is no problem now for x86_64.

Hongchen Zhang added 3 commits November 17, 2022 19:26
e_flags should be written to the output elf file,because it contain
processor-specific flags. For example: LoongArch use e_flags to
distinguish LP64/XLP32/LP32.

Signed-off-by: Hongchen Zhang <zhanghongchen@loongson.cn>
if one debug section's rela reference to __verbose section's symbol
(for example, pr_debug) and some of __verbose section's symbols are
included,then the __verbose section will be included.
But other symbols of __verbose may be not included,then the debug
section's relas reference to the not included symbols would result
into bad rela in the end output elf file.
So if the rela's symbol is not included,just delete that rela.

Signed-off-by: Hongchen Zhang <zhanghongchen@loongson.cn>
Let's do R_X86_64_32S judgment firstly, so other architectures
would run a little faster.

Signed-off-by: Hongchen Zhang <zhanghongchen@loongson.cn>
@ZhangHongchen1
Copy link
Author

ZhangHongchen1 commented Nov 17, 2022

Hi @jpoimboe
As I make the found flag check every symbol replace loop,a x86 problem triggerd:

create-diff-object: ERROR: static-local-moved-subfunction-issue-1054.ORIG.o: kpatch_replace_sections_syms: 1622: .orc_unwind_ip+0xc8: can't find replacement symbol for .init.text+711 reference: Success

The reason seems to bee the offset 711 is outside of .init.text section.
Add an exception for this condition?

@ZhangHongchen1 ZhangHongchen1 force-pushed the master branch 2 times, most recently from ff698b7 to 3c97552 Compare November 22, 2022 06:35
found flag is set to false outside the whole loop of section symbol
replacement. So if one replacement occur, later found check would
always be ok which is not what we expected.
So reset found to false before every section symbol replace.

Signed-off-by: Hongchen Zhang <zhanghongchen@loongson.cn>
@github-actions
Copy link

This PR has been open for 60 days with no activity and no assignee. It will be closed in 7 days unless a comment is added.

@github-actions github-actions bot added the stale label Aug 20, 2023
@github-actions
Copy link

This PR was closed because it was inactive for 7 days after being marked stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants