Page MenuHomeFreeBSD
Differential D51188

dtrace: fix symbol address resolving
ClosedPublic
Actions

Authored by zldrobit_gmail.com on Mon, Jul 7, 7:34 AM.
Tags
Referenced Files
Unknown Object (File)
Tue, Jul 22, 7:50 AM
Unknown Object (File)
Sun, Jul 20, 9:18 PM
Unknown Object (File)
Wed, Jul 16, 7:26 AM
Unknown Object (File)
Fri, Jul 11, 9:02 PM
Unknown Object (File)
Fri, Jul 11, 9:02 PM
Unknown Object (File)
Fri, Jul 11, 9:02 PM
Unknown Object (File)
Fri, Jul 11, 9:02 PM
Unknown Object (File)
Fri, Jul 11, 9:02 PM
View All 14 Files
Subscribers
avg
freebsd-dtrace-list
imp
markj

Details

Reviewers
gnn
markj
Group Reviewers
DTrace
Commits
rG12bef37a824c: dtrace: fix symbol address resolution
Summary

Dtrace assumes only ELF sections of type SHT_PROGBITS or SHT_NOBITS occupy memory space. However, sections with SHF_ALLOC flag also consume memory space. Moreover, the symbol address initialization skips symbols at the very beginning of a section.

Fix: Check section flag for calculating section offset, and disable the skipping at the beginning of a section.

PR: 288000

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

zldrobit_gmail.com created this revision.Mon, Jul 7, 7:34 AM
Herald added a reviewer: gnn. · View Herald TranscriptMon, Jul 7, 7:34 AM
Herald added a subscriber: imp. · View Herald Transcript
zldrobit_gmail.com requested review of this revision.Mon, Jul 7, 7:34 AM
avg added a subscriber: avg.Mon, Jul 7, 8:07 AM
avg added a comment.Mon, Jul 7, 1:03 PM

@zldrobit_gmail.com if you cannot submit the change for review from Git (e.g., using git-arc), can you update it with a diff created with -U999 option?

markj added a subscriber: markj.Mon, Jul 7, 2:45 PM

This change looks reasonable to me. I think the problem with st_value = 0 only applies to ET_REL KLDs.

cddl/contrib/opensolaris/lib/libdtrace/common/dt_module.c
162

Shouldn't we make the same change in dt_module_sysinit32()?

zldrobit_gmail.com updated this revision to Diff 158107.Mon, Jul 7, 3:51 PM

The patch is updated:

  1. Delete sym->st_value != 0 in dt_module_syminit32().
  2. Generate patch file with -U999 option.
zldrobit_gmail.com added a comment.Mon, Jul 7, 3:58 PM

@avg Sure, I recreated the patch with the -U999 option.

cddl/contrib/opensolaris/lib/libdtrace/common/dt_module.c
162

Thanks for your suggestion. I updated the patch accordingly. Plz check the new version.

markj accepted this revision as: markj.Mon, Jul 7, 4:56 PM

I'll commit this once I've gone through the test suite.

This revision is now accepted and ready to land.Mon, Jul 7, 4:56 PM
zldrobit_gmail.com updated this revision to Diff 158120.Mon, Jul 7, 6:20 PM
zldrobit_gmail.com marked an inline comment as not done.

@markj I found that we should also delete st_value = 0 in dt_module_symsort32/64(), otherwise dmp->dm_aslen would always be lesser than dmp->dm_asrsv by at least one. As a result, dt_module_symsort32/64() ignore the last element in dmp->dm_asmap and dt_module_symaddr32/64() wouldn't find it. I had to update the patch accordingly. Sorry for interrupting the test suite procedure.

This revision now requires review to proceed.Mon, Jul 7, 6:20 PM
markj added a comment.Mon, Jul 7, 6:41 PM
In D51188#1168819, @zldrobit_gmail.com wrote:

@markj I found that we should also delete st_value = 0 in dt_module_symsort32/64(), otherwise dmp->dm_aslen would always be lesser than dmp->dm_asrsv by at least one. As a result, dt_module_symsort32/64() ignore the last element in dmp->dm_asmap and dt_module_symaddr32/64() wouldn't find it. I had to update the patch accordingly. Sorry for interrupting the test suite procedure.

Good catch, thank you.

markj added a comment.Mon, Jul 7, 6:51 PM
In D51188#1168819, @zldrobit_gmail.com wrote:

@markj I found that we should also delete st_value = 0 in dt_module_symsort32/64(), otherwise dmp->dm_aslen would always be lesser than dmp->dm_asrsv by at least one. As a result, dt_module_symsort32/64() ignore the last element in dmp->dm_asmap and dt_module_symaddr32/64() wouldn't find it. I had to update the patch accordingly. Sorry for interrupting the test suite procedure.

It would be nice to refactor this ST_BIND != STB_LOCAL || sym->st_size != 0 check into a helper function, but this should be done in a separate patch.

This revision was not accepted when it landed; it landed in state Needs Review.Mon, Jul 7, 8:17 PM
Closed by commit rG12bef37a824c: dtrace: fix symbol address resolution (authored by zldrobit_gmail.com, committed by markj). · Explain Why
This revision was automatically updated to reflect the committed changes.
markj added a commit: rG12bef37a824c: dtrace: fix symbol address resolution.

Revision Contents
Changeset List

PathSize
cddl/
contrib/
opensolaris/
lib/
libdtrace/
common/
14 lines

Diff 158120

View Options

cddl/contrib/opensolaris/lib/libdtrace/common/dt_module.c

Loading...