From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4530 invoked by alias); 20 Mar 2012 19:26:45 -0000 Received: (qmail 4426 invoked by uid 22791); 20 Mar 2012 19:26:42 -0000 X-SWARE-Spam-Status: No, hits=-3.4 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,RCVD_IN_DNSWL_LOW,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail-vx0-f197.google.com (HELO mail-vx0-f197.google.com) (209.85.220.197) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 20 Mar 2012 19:26:27 +0000 Received: by vcbfo13 with SMTP id fo13so642163vcb.8 for ; Tue, 20 Mar 2012 12:26:26 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:reply-to:x-google-appengine-app-id:message-id:date :subject:from:to:cc:content-type:x-gm-message-state; bh=F+WGGEv4dEaY60YMeP9AA+72MZCYbMD7kH7LmD0ZLaM=; b=U/ykWjtoCiH8SlJpf0NQqjaqlv8zPUhVdCVhQvXRzCTlxfSFKdzv0uPMiy+FofyqVA KOoL2xqpofJO8Kvn2+yn8usPBRLLmywV4+p/8iHKhl7V4Fv9vMCPgdgorEcc1XlzeARC Jl86Y3cK+d2j9tPR+2AcWoXTRojlkSqz6kXU9mrrQfOExIMDovX1RmApcRYb2eG+q7Zm mKCfU85NaxU+XGpPO3U36IsQGUSmpO2l+ruFK04TYAy70klvo67ML04i+vT1yziKwRdG TFCBVx+I2ca0bTisk7oAiThqnTs68LON/uxSYvg/RtbLeyTlqr9mVDZ6zQ18QvG3s5qW j5aQ== Received: by 10.236.192.164 with SMTP id i24mr1608476yhn.8.1332271586853; Tue, 20 Mar 2012 12:26:26 -0700 (PDT) MIME-Version: 1.0 Received: by 10.236.192.164 with SMTP id i24mr1608454yhn.8.1332271586716; Tue, 20 Mar 2012 12:26:26 -0700 (PDT) Reply-To: tmsriram@google.com, eraman@google.com, davidxl@google.com, gcc-patches@gcc.gnu.org, reply@codereview-hr.appspotmail.com X-Google-Appengine-App-Id: s~codereview-hr Message-ID: <20cf305642d7abae8a04bbb1a61b@google.com> Date: Tue, 20 Mar 2012 19:26:00 -0000 Subject: Re: [google][4.6] Bug fixes to function reordering linker plugin to handle local and comdat functions. (issue 5851044) From: eraman@google.com To: tmsriram@google.com, davidxl@google.com Cc: gcc-patches@gcc.gnu.org, reply@codereview-hr.appspotmail.com Content-Type: text/plain; charset=ISO-8859-1; format=flowed; delsp=yes X-Gm-Message-State: ALoCoQmxzh9aLDFma6mOWOO2kD6HrSckcSYCyroyD1HntwDn2hGnxOXGwpZkOB/dimDJKy0hmpj5Z4tceK9awtbqLzYk3w3Eqr344YQD+/KtPqzWsNX/ZtMax36HokI6iZGssATipB14RMuSbCLVZtvd8VFjZlw6zyiC5NY5cTKDOTfUtLQbDoQ= X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2012-03/txt/msg01383.txt.bz2 http://codereview.appspot.com/5851044/diff/1/callgraph.c File callgraph.c (right): http://codereview.appspot.com/5851044/diff/1/callgraph.c#newcode513 callgraph.c:513: const int section_priority[] = {0, 3, 4, 2, 1}; Add a comment about section_priority http://codereview.appspot.com/5851044/diff/1/callgraph.c#newcode571 callgraph.c:571: if (section_priority[kept->section_type] Add an example that shows why we want to do that http://codereview.appspot.com/5851044/diff/1/callgraph.c#newcode655 callgraph.c:655: write_out_node (n_it->name, §ion_start[0], §ion_end[0]); In write_out_node, why take the function name and do a hash table lookup to get the section, instead of directly passing Section_id * in the caller. In all calls to write_out_node, you are in fact getting the name from the Section_id *. http://codereview.appspot.com/5851044/diff/1/callgraph.c#newcode674 callgraph.c:674: s_it->processed = 1; setting processed to 1 is redundant as it is already done in write_out_node. http://codereview.appspot.com/5851044/diff/1/callgraph.h File callgraph.h (right): http://codereview.appspot.com/5851044/diff/1/callgraph.h#newcode31 callgraph.h:31: void push_mm_ptr (void *ptr); push_allocated_ptr or save_allocated_ptr would be a better name. http://codereview.appspot.com/5851044/diff/1/callgraph.h#newcode48 callgraph.h:48: push_mm_ptr (list); It might be cleaner to create a wrapper around XNEW that calls push_mm_ptr after XNEW. Similar for malloc as well. http://codereview.appspot.com/5851044/