From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14983 invoked by alias); 21 Mar 2012 04:19:40 -0000 Received: (qmail 14974 invoked by uid 22791); 21 Mar 2012 04:19:38 -0000 X-SWARE-Spam-Status: No, hits=-3.0 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-qa0-f69.google.com (HELO mail-qa0-f69.google.com) (209.85.216.69) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 21 Mar 2012 04:19:25 +0000 Received: by qabj40 with SMTP id j40so581601qab.8 for ; Tue, 20 Mar 2012 21:19:25 -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=hZvEHNinZTU18z52tr0ZqHNqnPX7juNxsMtA134lxZk=; b=NUAAK/qOuxfwhN1pijklwbo9o1AegY/+Rz+nP+USCDOzJPeuLTmIpro1J/CB3UHVk9 9A0gPKp+eparBleA6l1RSGQhBjD9YrgDURMs5EhsqRbvRf+9uvcWK71hE0/75kUylw/s VnC/xkl7F0KXajqLroJ51ojYlBLbZav97PFoUX0z/Dw/f4lW23NmISFX6jrLS5xZXTh/ ZQrl6v5QQAQugDb64FcSheVAKHCgwCsn7d5e2jbAefL0rh6jkzggp6NDeWUDqeC0f+M9 uaWagFc9F4gCvHAj6NibUxRQ+2S+o9OHPszLa8O5AhzeHqvqpgel6LWeBd6dyQGlcJ5g yE+Q== Received: by 10.101.108.16 with SMTP id k16mr1090959anm.22.1332303565066; Tue, 20 Mar 2012 21:19:25 -0700 (PDT) MIME-Version: 1.0 Received: by 10.101.108.16 with SMTP id k16mr1090937anm.22.1332303564854; Tue, 20 Mar 2012 21:19:24 -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: <001636eee181b7576f04bbb918e1@google.com> Date: Wed, 21 Mar 2012 04:19:00 -0000 Subject: Re: [google][4.6] Bug fixes to function reordering linker plugin to handle local and comdat functions. (issue 5851044) From: tmsriram@google.com To: eraman@google.com, davidxl@google.com Cc: gcc-patches@gcc.gnu.org, davidxl@google.com, reply@codereview-hr.appspotmail.com Content-Type: text/plain; charset=ISO-8859-1; format=flowed; delsp=yes X-Gm-Message-State: ALoCoQn5gVa68LKrJNE0dLigNZ2X+LjXji/h7ALLGjFYhtjFlmyuTmeqygeQXi9JuiUVkCRYMJNfDvsJwpsDa6jqCcQG0KTJmckVb/X1Thili6A6oZAa3ldA/9QojQY+ykxKynlkBHIRtdEW62dLgjQ/3gq+sq8BaKYsHDMjdOwEha+EXPBWGZY= 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/msg01409.txt.bz2 On 2012/03/20 19:26:26, eraman wrote: > 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 Done. > 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 I added a comment. > 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 *. That is not true, I pass the Node * in the first two calls. What you said holds for the other calls. So, I made this more efficient. > 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. Done. > 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. Done. > 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. Done. http://codereview.appspot.com/5851044/