From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1124 invoked by alias); 22 Aug 2012 17:59:33 -0000 Received: (qmail 1112 invoked by uid 22791); 22 Aug 2012 17:59:30 -0000 X-SWARE-Spam-Status: No, hits=-5.8 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,KHOP_RCVD_TRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail-lb0-f169.google.com (HELO mail-lb0-f169.google.com) (209.85.217.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 22 Aug 2012 17:59:04 +0000 Received: by lbon3 with SMTP id n3so850006lbo.0 for ; Wed, 22 Aug 2012 10:59:02 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:x-system-of-record:x-gm-message-state; bh=F02CDQz4bR8g/LB5JIDFY+jzdN7pWPElOA3U1KFSIXg=; b=fqTndz779dfhlcKI7jqCl6rJJG2ela+tkoOwcXlmKM31pZfT9EGGAt0UykFyCRenQC oWcIMBEHDlstWXqpvZFF8W6T/yC1T4SLucYVIvkVubACUFgA7j+ryk6KFEeLc3zjHUXp 3YH1Z0O3+prIa+QyI+DKS5EokUs+p8L3xqNQgj/1+Bi+G+WkWuCltk1NW3IzsOOPJzQk ZGjJ+U3ACfd8dlYTZlZh6KuMztEL8cF8nWcjCgR8PnSAsVx2Tv0Bnwjzisp+mkqm4NsK 2L3HudLhHoIqkT8Qf8m9Srv1meXDM0ovuIn5iMaeNstW9zWCSgVnACMFsN484fuITK99 xM0Q== Received: by 10.112.54.6 with SMTP id f6mr9849674lbp.89.1345658342775; Wed, 22 Aug 2012 10:59:02 -0700 (PDT) MIME-Version: 1.0 Received: by 10.112.54.6 with SMTP id f6mr9849666lbp.89.1345658342589; Wed, 22 Aug 2012 10:59:02 -0700 (PDT) Received: by 10.112.83.99 with HTTP; Wed, 22 Aug 2012 10:59:02 -0700 (PDT) In-Reply-To: References: Date: Wed, 22 Aug 2012 21:41:00 -0000 Message-ID: Subject: Re: [patch] Gold linker patch to provide plugin support for mapping some text sections to an unique ELF segment. From: Ian Lance Taylor To: Sriraman Tallam Cc: Cary Coutant , binutils Content-Type: text/plain; charset=ISO-8859-1 X-System-Of-Record: true X-Gm-Message-State: ALoCoQmEehRgrrzaie1IS2al6HTQVJ5gg10HnTToXicHQeEzLYKKYUNC+bxBoKAwTsX716htF4XvFnp9Fv8CWirLQYA4AMqNuj8P9NqFzgS4UA0mRl9ZYW5EYdpXwekjR44xa6gGS7sARRrJFcLemtnaas58hU94TGMzjCDCuKSkaEgiOhFUYrpSBzfoe/+YtLmGGQPp1m8Z X-IsSubscribed: yes Mailing-List: contact binutils-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: binutils-owner@sourceware.org X-SW-Source: 2012-08/txt/msg00389.txt.bz2 On Thu, Aug 9, 2012 at 2:32 PM, Sriraman Tallam wrote: > > * gold.cc (queue_middle_tasks): Call layout again when unique > segments for sections is desired. > * layout.cc (Layout::Layout): Initialize new members. > (Layout::layout): Make output section for mapping to a unique segment. > (Layout::attach_allocated_section_to_segment): Make unique segment for > output sections marked so. > (Layout::segment_precedes): Check for unique segments when sorting. > * layout.h (Layout::Unique_segment_info): New struct. > (Layout::Section_segment_map): New typedef. > (Layout::get_section_segment_map): New function. > (Layout::is_unique_segment_for_sections_specified): New function. > (Layout::set_unique_segment_for_sections_specified): New function. > (Layout::unique_segment_for_sections_specified_): New member. > (Layout::section_segment_map_): New member. > * object.cc (Sized_relobj_file::do_layout): > Rename is_gc_pass_one to is_pass_one. > Rename is_gc_pass_two to is_pass_two. > Rename is_gc_or_icf to is_two_pass. > Check for which pass based on whether symbols data is present. > Make it two pass when unique segments for sections is desired. > * output.cc (Output_section::Output_section): Initialize new > members. > * output.h (Output_section::is_unique_segment): New function. > (Output_section::set_is_unique_segment): New function. > (Output_section::is_unique_segment_): New member. > (Output_section::extra_segment_flags): New function. > (Output_section::set_extra_segment_flags): New function. > (Output_section::extra_segment_flags_): New member. > (Output_section::segment_alignment): New function. > (Output_section::set_segment_alignment): New function. > (Output_section::segment_alignment_): New member. > (Output_segment::Output_segment): Initialize is_unique_segment_. > (Output_segment::is_unique_segment): New function. > (Output_segment::set_is_unique_segment): New function. > (Output_segment::is_unique_segment_): New member. > * plugin.cc (allow_unique_segment_for_sections): New function. > (unique_segment_for_sections): New function. > (Plugin::load): Add new functions to transfer vector. > * Makefile.am (plugin_final_layout.readelf.stdout): Add readelf output. > * Makefile.in: Regenerate. > * testsuite/plugin_final_layout.sh: Check if unique segment > functionality works. > * testsuite/plugin_section_order.c (onload): Check if new interfaces > are available. > (allow_unique_segment_for_sections): New global. > (unique_segment_for_sections): New global. > (claim_file_hook): Call allow_unique_segment_for_sections. > (all_symbols_read_hook): Call unique_segment_for_sections. > > > * plugin-api.h (ld_plugin_allow_unique_segment_for_sections): > New interface. > (ld_plugin_unique_segment_for_sections): New interface. > (LDPT_ALLOW_UNIQUE_SEGMENT_FOR_SECTIONS): New enum val. > (LDPT_UNIQUE_SEGMENT_FOR_SECTIONS): New enum val. > (tv_allow_unique_segment_for_sections): New member. > (tv_unique_segment_for_sections): New member. > + if (it != this->section_segment_map_.end()) > + { > + // We know the name of the output section, directly call > + // get_output_section here by-passing choose_output_section. > + elfcpp::Elf_Xword flags = shdr.get_sh_flags(); Please invert this conditional, so the common and shorter case appears. That is if (it == this->section_segment_map_.end()) os = this->choose_output_section(...); else { ... } > + // We know the name of the output section, directly call > + // get_output_section here by-passing choose_output_section. > + elfcpp::Elf_Xword flags = shdr.get_sh_flags(); > + // Some flags in the input section should not be automatically > + // copied to the output section. > + flags &= ~ (elfcpp::SHF_INFO_LINK > + | elfcpp::SHF_GROUP > + | elfcpp::SHF_MERGE > + | elfcpp::SHF_STRINGS); > + > + // We only clear the SHF_LINK_ORDER flag in for > + // a non-relocatable link. > + if (!parameters->options().relocatable()) > + flags &= ~elfcpp::SHF_LINK_ORDER; This code is too finicky to have two copies. Please refactor into a common function one way or another. > + os_name = this->namepool_.add_with_length(os_name, strlen(os_name), > + true, &name_key); Don't call add_with_length(x, strlen(x), ...). Just call add(). > + // If this segment is marked unique, skip. This comment adds nothing to the code. > + if (os->segment_alignment()) os->segment_alignment is not a bool: write os->segment_alignment() != 0. > + typedef struct > + { > + // Identifier for the Segment. ELF Segments dont have names. > + const char* name; > + // Segment flags. > + uint64_t flags; > + uint64_t align; > + } Unique_segment_info; This is C++, not C. Don't write typedef struct { } S. Just write struct S { }. The align field should have a comment. The flags field comment should say something like "Additional segment flags." > + Section_segment_map& > + get_section_segment_map() > + { return this->section_segment_map_; } gold doesn't use non-const references as function parameter or return types. This should be a pointer. > + gold_assert (sd != NULL); Remove space before left parenthesis. Thanks for the cleanups here. > + const unsigned char* pnamesu = (is_two_pass) > ? gc_sd->section_names_data > : sd->section_names->data(); It's not new in this patch, but the parenthesization here is wrong. It should be ... = (is_two_pass ? ... : ...); The ? and : should be lined up under the start of the condition (i.e., under the 'i'). L + gold_assert(!(is_two_pass) || reloc_sections.empty()); No need to parenthesize is_two_pass. > + uint64_t extra_segment_flags() const > + { return extra_segment_flags_; } > + > + void > + set_extra_segment_flags(uint64_t flags) > + { extra_segment_flags_ = flags; } > + > + uint64_t segment_alignment() const > + { return segment_alignment_; } > + > + void > + set_segment_alignment(uint64_t align) > + { segment_alignment_ = align; } This functions are all missing "this->". > + bool > + is_unique_segment() const > + { return is_unique_segment_; } > + > + // Mark segment as unique, happens when linker plugins request that > + // certain input sections be mapped to unique segments. > + void > + set_is_unique_segment() > + { this->is_unique_segment_ = true; } Missing this-> here too. > + Layout* layout = parameters->options().plugins()->layout(); > + gold_assert (layout != NULL); Somewhere here you should ensure that the plugin called the LDPT_ALLOW_UNIQUE_SEGMENT_FOR_SECTIONS entry point. > + Layout::Unique_segment_info* s = static_cast > + (malloc(sizeof(Layout::Unique_segment_info))); This is C++. You mean Layout::Unique_segment_info* s = new Unique_segment_info; > + section_segment_map[secn_id] = s; Rather than have Layout return a pointer to its private data member, I would prefer to see a Layout method that saves this value. And that Layout method should assert unique_segment_for_sections_specified_. > +check_DATA += plugin_final_layout.stdout plugin_final_layout.readelf.stdout Stick to a single '.' per fliename. plugin_final_layout_readelf.stdout is fine here. > With readelf -l, an ELF Section to Segment mapping is printed as : readelf can be a bit unpredictable: I recommend using the -W option as well. > +/* The linker's interface for specifying that a subset of sections is > + to be mapped to a unique segment. This should be invoked before > + unique_segment_for_sections, preferably in the claim_file handler. */ This is a "must", not a "should". How about something along the lines of "If the plugin wants to call unique_segment_for_sections, it must call this function from a claim_file handler or when it is first loaded." Sorry for the slow review. Ian