Split segments and record segment offsets. #2940

snehasish · 2018-04-18T16:31:05Z

This change changes the modules.log format to provide a separate entry
for each segment instead of combining entries for contiguous segments
mapped from each image. The module file version is incremented to 4 and
adds a new field called offseti after the "entry" field.

The file offset information for unix is recorded in
module_add_segment_data and propagated to drmodtrack to be written out
with each segment.

This change changes the modules.log format to provide a separate entry for each segment instead of combining entries for contiguous segments mapped from each image. The module file version is incremented to 4 and adds a new field called offseti after the "entry" field. The file offset information for unix is recorded in module_add_segment_data and propagated to drmodtrack to be written out with each segment.

Do not set the offset field for WINDOWS.

derekbruening · 2018-04-18T18:36:10Z

ext/drcovlib/drcovlib.h

@@ -249,6 +249,10 @@ typedef struct _drmodtrack_info_t {
     * passed to drmodtrack_offline_lookup().
     */
    uint index;
+    /**
+     * The offset of this segment from the beginnig of this backing file.


"beginning"

derekbruening · 2018-04-18T18:38:54Z

core/unix/module_elf.c

@@ -539,7 +539,8 @@ module_walk_program_headers(app_pc base, size_t view_size, bool at_map, bool dyn
                                            (app_pc) prog_hdr->p_vaddr + load_delta,
                                            prog_hdr->p_memsz,
                                            module_segment_prot_to_osprot(prog_hdr),
-                                            prog_hdr->p_align, false/*!shared*/);
+                                            prog_hdr->p_align, false/*!shared*/,
+                                            prog_hdr->p_offset);


The call in module_macho.c needs to be updated as well. We removed Mac from Travis on PR's because the delay was too long to get a machine but it does run post-commit to master and this will fail to build there.

derekbruening · 2018-04-18T18:40:53Z

core/lib/instrument_api.h

@@ -2918,6 +2918,7 @@ typedef struct _module_segment_data_t {
    app_pc start; /**< Start address of the segment, page-aligned backward. */
    app_pc end;   /**< End address of the segment, page-aligned forward. */
    uint prot;    /**< Protection attributes of the segment */
+    uint64 offset; /**< Offset of the segment from the beginning of the backing file */


(We allocate this struct, so appending to the end should be fine compatibility-wise.)

Acknowledged.

derekbruening · 2018-04-18T18:50:05Z

ext/drcovlib/modules.c

                if (dr_sscanf(buf, "%u, %u, "PIFX", "PIFX", "PIFX", ",
                              &mod_id, &info->mod[i].containing_id,
                              &info->mod[i].base, &end, &info->mod[i].entry) != 5 ||
                    mod_id != i)
                    goto read_error;
                buf = skip_commas_and_spaces(buf, 5);
+            } else { // version == 4
+                if (dr_sscanf(buf, "%u, %u, "PIFX", "PIFX", "PIFX", "PIFX", ",


This is the wrong scanf type: PIFX is pointer-sized so for 32-bit it will be 32 bits. Use HEX64_FORMAT_STRING.

derekbruening · 2018-04-18T18:52:43Z

ext/drcovlib/modules.c

@@ -725,6 +734,7 @@ drmodtrack_offline_lookup(void *handle, uint index, OUT drmodtrack_info_t *out)
    out->timestamp = info->mod[index].timestamp;
 #endif
    out->custom = info->mod[index].custom;
+    out->offset = info->mod[index].offset;


This will overflow and corrupt memory for clients using the older struct. Use a struct_size compatibility check like the one below for index (seems good to move it down below the index set as well to keep it in order).

derekbruening · 2018-04-18T18:54:39Z

ext/drcovlib/modules.c

                if (dr_sscanf(buf, "%u, %u, "PIFX", "PIFX", "PIFX", ",
                              &mod_id, &info->mod[i].containing_id,
                              &info->mod[i].base, &end, &info->mod[i].entry) != 5 ||
                    mod_id != i)
                    goto read_error;
                buf = skip_commas_and_spaces(buf, 5);
+            } else { // version == 4


Bug: removing the else from the 3 above means version 2 will enter here -- seems best to have the if for 3 be an else if.

derekbruening · 2018-04-18T18:56:09Z

ext/drcovlib/modules.c

@@ -466,6 +461,11 @@ module_table_entry_print(module_entry_t *entry, char *buf, size_t size)
 #endif
    read_entry.path = full_path;
    read_entry.custom = entry->custom;
+#ifndef WINDOWS
+    // For unices we record the physical offset from the backing file. Each
+    // module entry only has a sigle segment so we grab index 0 of the array.


derekbruening · 2018-04-18T18:57:52Z

ext/drcovlib/modules.c

@@ -413,9 +408,9 @@ static int
 module_read_entry_print(module_read_entry_t *entry, uint idx, char *buf, size_t size)
 {
    int len, total_len = 0;
-    len = dr_snprintf(buf, size, "%3u, %3u, " PFX ", " PFX ", " PFX ", ",
+    len = dr_snprintf(buf, size, "%3u, %3u, " PFX ", " PFX ", " PFX ", " PFX ", ",


PFX is the wrong code: it is pointer-sized while offset is always 64 bits. Use ZHEX64_FORMAT_STRING.

derekbruening · 2018-04-18T19:01:29Z

ext/drcovlib/modules.c

+#ifndef WINDOWS
+    // For unices we record the physical offset from the backing file. Each
+    // module entry only has a sigle segment so we grab index 0 of the array.
+    read_entry.offset = entry->data->segments[0].offset;


I don't think this is correct: entry->data points at DR's module_data_t and each segment entry here points at the same data structure. module_entry_t needs to store either the offset or the segment index (or I guess you could compute the segment index here by assuming they're contiguous and in order).

derekbruening · 2018-04-18T19:05:00Z

ext/drcovlib/modules.c

+#ifndef WINDOWS
+    // For unices we record the physical offset from the backing file. Each
+    // module entry only has a sigle segment so we grab index 0 of the array.
+    read_entry.offset = entry->data->segments[0].offset;


Maybe it's worth adding code to suite/tests/client-interface/drmodtrack-test.dll.cpp to test that the offsets match the DR fields (or even better the /proc/self/maps fields on Linux but that is more work).

I'm unsure what is implied by "match the DR fields". For example I can get info.offset here (after checking info.struct_size) --
https://github.com/DynamoRIO/dynamorio/blob/master/suite/tests/client-interface/drmodtrack-test.dll.cpp#L134
but I don't understand what I can compare it with (apart from parsing proc/self/maps)?

I mean using DR's module list while the process is live here to get module info and compare it to drmodtrack's: e.g., using dr_module_iterator_start() or dr_lookup_module().

derekbruening · 2018-04-18T19:06:42Z

core/lib/instrument.c

@@ -1811,6 +1811,7 @@ create_and_initialize_module_data(app_pc start, app_pc end, app_pc entry_point,
            copy->segments[i].start = os_segments[i].start;


Also, please add a feature entry to https://github.com/DynamoRIO/dynamorio/blob/master/api/docs/release.dox#L230

Fixed issues.

Updated documentation.

fhahn · 2018-04-19T11:49:39Z

run arm tests

Extend the test in client-interface/drmodtrack-test.dll.cpp to check whether the offset recorded by drmodtrack match the offset for the segment in DRs module data. Also fixed lint issue.

Fix test failure on Windows where the struct fields for segments don't exist.

snehasish · 2018-04-19T16:59:37Z

I've added the test, fixed lint issues and fixed the build issue for Windows. Please take another look. Thanks.

fhahn · 2018-04-19T17:11:36Z

add to whitelist

derekbruening

Just some style nits

derekbruening · 2018-04-19T21:49:10Z

api/docs/release.dox

@@ -229,6 +229,8 @@ Further non-compatibility-affecting changes include:
   of built basic blocks.
 - Added drreg_reservation_info_ex(), drreg_statelessly_restore_app_value(),
   and drreg_is_instr_spill_or_restore().
+ - Module tracking allocates an entry per segment for each loaded module.


Please include drmodtrack so we know which library/component this is: the core API provides module information as well.

derekbruening · 2018-04-19T21:51:00Z

api/docs/release.dox

@@ -229,6 +229,8 @@ Further non-compatibility-affecting changes include:
   of built basic blocks.
 - Added drreg_reservation_info_ex(), drreg_statelessly_restore_app_value(),
   and drreg_is_instr_spill_or_restore().
+ - Module tracking allocates an entry per segment for each loaded module.
+   File offsets for each segment are recorded in modules.log.


Please include drcachesim (and UNIX) to give context on which tool/component this is referring to.

derekbruening · 2018-04-19T21:52:15Z

api/docs/release.dox

@@ -229,6 +229,8 @@ Further non-compatibility-affecting changes include:
   of built basic blocks.
 - Added drreg_reservation_info_ex(), drreg_statelessly_restore_app_value(),
   and drreg_is_instr_spill_or_restore().
+ - Module tracking allocates an entry per segment for each loaded module.
+   File offsets for each segment are recorded in modules.log.


Please also mention that #module_segment_data_t now includes the file offset on UNIX.

derekbruening · 2018-04-19T21:58:27Z

suite/tests/client-interface/drmodtrack-test.dll.cpp

@@ -132,6 +133,19 @@ event_exit(void)
        CHECK(((app_pc)info.custom) == info.start ||
              info.containing_index != i, "custom field doesn't match");
        CHECK(info.index == i, "index field doesn't match");
+#ifndef WINDOWS
+        if(info.struct_size > offsetof(drmodtrack_info_t, offset)) {


style: if (

derekbruening · 2018-04-19T21:58:54Z

suite/tests/client-interface/drmodtrack-test.dll.cpp

+#ifndef WINDOWS
+        if(info.struct_size > offsetof(drmodtrack_info_t, offset)) {
+            module_data_t * data = dr_lookup_module(info.start);
+            for(uint j = 0; j < data->num_segments; j++) {


style: for (

derekbruening · 2018-04-19T21:59:22Z

suite/tests/client-interface/drmodtrack-test.dll.cpp

+            module_data_t * data = dr_lookup_module(info.start);
+            for(uint j = 0; j < data->num_segments; j++) {
+                module_segment_data_t *seg = data->segments + j;
+                if(seg->start == info.start) {


style: if (

derekbruening · 2018-04-19T22:01:01Z

suite/tests/client-interface/drmodtrack-test.dll.cpp

+                module_segment_data_t *seg = data->segments + j;
+                if(seg->start == info.start) {
+                    CHECK(seg->offset == info.offset,
+                        "Module data offset and drmodtrack offset don't match");


style nit: Indent looks off (should line up with seg after the paren.)

derekbruening · 2018-04-19T22:02:13Z

ext/drcovlib/modules.c

@@ -663,13 +669,23 @@ drmodtrack_offline_read(file_t file, const char *map, OUT const char **next_line
                    goto read_error;
                info->mod[i].containing_id = mod_id;
                buf = skip_commas_and_spaces(buf, 4);
-            } else {
+            }
+            else if (version == 3) {


minor nit: since already changing some other things I would suggest putting the else on the prior line for consistency w/ the surrounding if-else chains

Updated release docs and fixed style issues.

…h/dynamorio into i2939-phys-offset-split-segment

…nt offsets (#2940)" This reverts commit 8c90e2d as it pushes the module count for drcachesim offline traces beyond the limit in the module index bitfield (#2956). Issue: #2006, #2939, #2956.

…nt offsets (#2940)" (#2963) This reverts commit 8c90e2d as it pushes the module count for drcachesim offline traces beyond the limit in the module index bitfield (#2956) for some apps. Issue: #2006, #2939, #2956.

This reverts commit 48db566 which reverted the changes in PR DynamoRIO#2940. The changes pushed caused some apps to overflow the modidx field (issue DynamoRIO#2956). PR DynamoRIO#2969 increased the width of the modidx field. We can now safely revert the revert.

This reverts commit 48db566 which reverted the changes in PR #2940. The changes pushed caused some apps to overflow the modidx field (issue #2956). PR #2969 increased the width of the modidx field. We can now safely revert the revert. Issue: #2006, #2939, #2956

PR #2940 and #2973 added an offset field to drmodtrack for #2939, but they ifdef-ed the field in some places but not others, resulting in uninitialized output. Since it's already locked into the interface, we always set it to 0 here and include it in internal structures to fix the problem. Tested on drmodtrack-test where the fields were manually confirmed to no longer contain bogus values, and where the test doesn't fail when that masked this bug before. Issue: #2939, #4474, #4777 Fixes #4777

PR #2940 and #2973 added an offset field to drmodtrack for #2939, but they ifdef-ed the field in some places but not others, resulting in uninitialized output. Since it's already locked into the interface, we always set it to 0 here and include it in internal structures to fix the problem. Tested on drmodtrack-test where the fields were manually confirmed to no longer contain bogus values, and where the test doesn't fail when i#4474 adds a new field that shifts the test buffer to avoid the happens-to-match scenario that masked this bug in that test before. Issue: #2939, #4474, #4777 Fixes #4777

snehasish added 2 commits April 18, 2018 11:33

Split segments and record segment offsets.

a1b810f

Do not set the offset field for WINDOWS.

derekbruening reviewed Apr 18, 2018

View reviewed changes

derekbruening mentioned this pull request Apr 18, 2018

burst_threadfilter test failing with "Cannot correctly handle a received signal" #2941

Closed

snehasish added 2 commits April 18, 2018 17:05

Split segments and record segment offsets.

ef2981d

Fixed issues.

Split segments and record segment offsets.

58e9ba8

Updated documentation.

snehasish added 2 commits April 19, 2018 11:48

Split segments and record segment offsets.

6c8aec6

Extend the test in client-interface/drmodtrack-test.dll.cpp to check whether the offset recorded by drmodtrack match the offset for the segment in DRs module data. Also fixed lint issue.

Split segments and record segment offsets.

b412378

Fix test failure on Windows where the struct fields for segments don't exist.

Merge branch 'master' into i2939-phys-offset-split-segment

eecf69f

derekbruening reviewed Apr 19, 2018

View reviewed changes

snehasish added 2 commits April 19, 2018 18:31

Split segments and record segment offsets.

5658f38

Updated release docs and fixed style issues.

Merge branch 'i2939-phys-offset-split-segment' of github.com:snehasis…

ab5f181

…h/dynamorio into i2939-phys-offset-split-segment

derekbruening approved these changes Apr 19, 2018

View reviewed changes

derekbruening merged commit 8c90e2d into DynamoRIO:master Apr 19, 2018

derekbruening mentioned this pull request Apr 20, 2018

Record the physical offset of each segment in modules.log #2939

Closed

snehasish mentioned this pull request Apr 24, 2018

modidx field in offline_entry_t overflows for split segments #2956

Closed

snehasish mentioned this pull request Apr 29, 2018

Revert "Revert (#2940)" #2973

Merged

derekbruening mentioned this pull request Mar 10, 2021

drmodtrack offset field has uninitialized values on Windows #4777

Closed

derekbruening mentioned this pull request Mar 10, 2021

i#4777: Fix uninitialized drmodtrack offset on Windows #4778

Merged

		@@ -1811,6 +1811,7 @@ create_and_initialize_module_data(app_pc start, app_pc end, app_pc entry_point,
		copy->segments[i].start = os_segments[i].start;

Split segments and record segment offsets. #2940

Split segments and record segment offsets. #2940

Conversation

snehasish commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhahn commented Apr 19, 2018

snehasish commented Apr 19, 2018

fhahn commented Apr 19, 2018

derekbruening left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment