New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 820290 link

Starred by 1 user

Issue metadata

Status: Untriaged
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug
Build-Toolchain



Sign in to add a comment

sys-power/dptf fails to build when SHT_RELR sections are enabled

Project Member Reported by rahulchaudhry@chromium.org, Mar 8 2018

Issue description

I am testing support for SHT_RELR sections in chromium os. SHT_RELR sections are used to compactly store all the R_X86_64_RELATIVE relocations in a PIE binary or a shared object. For more background, see the official proposal here: https://groups.google.com/forum/#!topic/generic-abi/bX460iggiKg

SHT_RELR sections do not support relative relocations with odd offsets.
This results in sys-power/dptf failing to build when SHT_RELR sections are enabled.

Steps to reproduce:

1. setup_board --board=chell
2. build_packages --board=chell
3. cherry pick https://chromium-review.googlesource.com/733802
4. sudo emerge cross-x86_64-cros-linux-gnu/binutils
5. emerge-chell sys-power/dptf

fails with:

usr/x86_64-pc-linux-gnu/x86_64-cros-linux-gnu/binutils-bin/2.27.0/ld.gold.real: internal error in shrink_relocs, at output.h:2527
clang-6.0: error: linker command failed with exit code 1 (use -v to see invocation)

when linking 'esif_ufd' binary.

The relevant assert in gold linker is just checking that the offset is not odd:

2526         // Odd addresses are not supported in SHT_RELR.
2527         gold_assert(current%2 == 0);

 
Cc: brian.b...@intel.com
Digging deeper, the relocation containing odd offset is in the .../ESIF_UF/Sources/esif_uf_trace.o object.
It is pointing to the data segment. The corresponding global is defined in esif_uf_trace.c source file as:

struct esif_tracelevel_s g_traceinfo[] = {
        {"FATAL",       ESIF_TRACELEVEL_FATAL,  ESIF_TRACEMASK_ALL,             ESIF_TRACEROUTE_EVENTLOG},
        {"ERROR",       ESIF_TRACELEVEL_ERROR,  ESIF_TRACEMASK_ALL,             ESIF_TRACEROUTE_EVENTLOG},
        {"WARNING",     ESIF_TRACELEVEL_WARN,   ESIF_TRACEMASK_ALL,             ESIF_TRACEROUTE_DEBUGGER},
        {"INFO",        ESIF_TRACELEVEL_INFO,   ESIF_TRACEMASK_ALL,             ESIF_TRACEROUTE_DEBUGGER},
        {"DEBUG",       ESIF_TRACELEVEL_DEBUG,  ESIF_TRACEMASK_ALL,             ESIF_TRACEROUTE_DEBUGGER},
};

Further, struct esif_tracelevel_s is defined in esif_uf_trace.h as:

#pragma pack(push,1)    
/* Trace Level Data */
struct esif_tracelevel_s {
        const char *            label;          // label, i.e. "ERROR"
        int                     level;          // level, i.e, 1
        esif_tracemask_t        modules;        // active module bitmask
        esif_traceroute_t       routes;         // active routes bitmask
};
#pragma pack(pop)

And esif_tracemask_t/esif_traceroute_t are defined in the same file as:

/* Trace Module and Route Masks */
typedef u32             esif_tracemask_t;
typedef u8              esif_traceroute_t;


In the struct esif_tracelevel_s, the four fields are {8, 4, 4, 1} bytes, for a total of 17 bytes.
The #pragma pack(push,1) makes instances of this struct 1-byte aligned.
Thus, in the g_traceinfo[] global array of struct esif_tracelevel_s, the second entry starts exactly
17 bytes ahead of the first entry, ensuring that one of them will be starting at an odd address.

The relative relocation in question is pointing to the label field, which is the first field in the struct.
This is the source for odd offsets in the relocation table.

Possible solutions I can think of:

1. Remove #pragma pack(push,1) from struct esif_tracelevel_s. Is this really necessary? As far as I can see, there is one array of this struct in esif_uf_trace.c containing only 5 entries. Removing the pragma increases the size of each entry in the array from 17 bytes to 24 bytes.

2. Change the pragma to specify 2 byte alignment with #pragma pack(push,2). This increases the size of each entry in the array from 17 bytes to 18 bytes, and ensures that each entry starts at an even address.

3. Implement support in gold linker for relative relocations with odd offsets. Theoretically, such relocations can be supported by simply excluding them from the .relr.dyn section (keeping them in the .rela.dyn section).

I would like to avoid implementing (3), since odd offsets in relocations are pretty rare, and even when they do occur, like in this case, they seem to be accidental. It is better to fix the few odd cases resulting in odd offsets than to add general support for odd offsets in the linker.

Thanks for the investigation. #1 should work. We will do some testing to make sure that traces still work as expected, then will upload a fix. 
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/bdfbbc6ad2629c3a6eb8301e10407bbf5a628d7f

commit bdfbbc6ad2629c3a6eb8301e10407bbf5a628d7f
Author: Rahul Chaudhry <rahulchaudhry@chromium.org>
Date: Fri Mar 16 04:23:28 2018

sys-power/dptf: append '--no-experimental-use-relr' to ldflags.

This is the default behavior of gold linker right now, so this CL does
not change any behavior. We are preparing for SHT_RELR sections to be
enabled by default in gold.

This change can be reverted once the real fix for http://crbug.com/820290
lands from upstream.

BUG=chromium:820290
TEST='emerge-chell sys-power/dptf' works with SHT_RELR sections enabled in gold.

Change-Id: I2afa7bdc28e0339e5b0355773311dec4d6b2a072
Reviewed-on: https://chromium-review.googlesource.com/965141
Commit-Ready: Rahul Chaudhry <rahulchaudhry@chromium.org>
Tested-by: Rahul Chaudhry <rahulchaudhry@chromium.org>
Reviewed-by: Manoj Gupta <manojgupta@chromium.org>
Reviewed-by: Brian Bian <brian.bian@intel.com>

[rename] https://crrev.com/bdfbbc6ad2629c3a6eb8301e10407bbf5a628d7f/sys-power/dptf/dptf-8.4.10400-r2.ebuild

Project Member

Comment 6 by bugdroid1@chromium.org, Jun 6 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/5f3a23d0cc2858b5008d9fd343cbf8af818e8514

commit 5f3a23d0cc2858b5008d9fd343cbf8af818e8514
Author: Brian Bian <brian.bian@intel.com>
Date: Wed Jun 06 04:28:39 2018

Update ebuild file for DPTF release 8.4.10401

Maintenance release with a few minor bug fixes:
1. Fixes to Critical Policy to enable the new UI (no functional
changes).
2. Fix alignment issuse found in tracing ( http://crbug.com/920290 ).
3. Suppress sensor polling/trace messages when sensors are not enabled.

BUG=chromium:820290
TEST=None

Change-Id: I333cd3371041656312dc739754c6178d08b0e5b3
Signed-off-by: Brian Bian <brian.bian@intel.com>
Reviewed-on: https://chromium-review.googlesource.com/1068216
Commit-Ready: Sumeet R Pawnikar <sumeet.r.pawnikar@intel.com>
Commit-Ready: Brian Bian <brian.bian@intel.corp-partner.google.com>
Tested-by: Brian Bian <brian.bian@intel.corp-partner.google.com>
Reviewed-by: Sumeet R Pawnikar <sumeet.r.pawnikar@intel.com>
Reviewed-by: Brian Bian <brian.bian@intel.corp-partner.google.com>
Reviewed-by: Duncan Laurie <dlaurie@google.com>

[rename] https://crrev.com/5f3a23d0cc2858b5008d9fd343cbf8af818e8514/sys-power/dptf/dptf-8.4.10401-r1.ebuild
[modify] https://crrev.com/5f3a23d0cc2858b5008d9fd343cbf8af818e8514/sys-power/dptf/Manifest

Sign in to add a comment