From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7100 invoked by alias); 3 Jan 2013 21:36:36 -0000 Received: (qmail 7043 invoked by uid 22791); 3 Jan 2013 21:36:35 -0000 X-SWARE-Spam-Status: No, hits=-6.3 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 03 Jan 2013 21:36:28 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r03LaNrC010042 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 3 Jan 2013 16:36:23 -0500 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r03LaLFp022583 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Thu, 3 Jan 2013 16:36:22 -0500 From: Tom Tromey To: Hui Zhu Cc: Hui Zhu , gdb-patches@sourceware.org Subject: Re: [PATCH] Add CTF support to GDB [1/4] Add "-ctf" to tsave command References: <50AC3217.6040608@mentor.com> <878v9k5g46.fsf@fleche.redhat.com> Date: Thu, 03 Jan 2013 21:36:00 -0000 In-Reply-To: (Hui Zhu's message of "Fri, 14 Dec 2012 19:36:52 +0800") Message-ID: <87hamy0x0q.fsf@fleche.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2.91 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2013-01/txt/msg00058.txt.bz2 >>>>> "Hui" == Hui Zhu writes: Hui> +struct ctf_save_collect_s Hui> +{ Hui> + struct ctf_save_collect_s *next; Hui> + char *str; Hui> + char *ctf_str; Hui> + int align_size; Hui> + struct expression *expr; Hui> + struct type *type; Hui> + int is_ret; Hui> +}; >> Like Hafiz said -- comments would be nice. Hui> I added some comments in the new patches. I looked at the new patches and did not see comments. For example, I looked at this struct I quoted above. Every new structure, field, and function ought to have a comment. Hui> + case TYPE_CODE_ARRAY: Hui> + for (; TYPE_CODE (type) == TYPE_CODE_ARRAY; Hui> + type = TYPE_TARGET_TYPE (type)) Hui> + ; Tom> You probably want some check_typedef calls in there. Hui> Because typedef will be handle as a type in this part, so this part Hui> doesn't need check_typedef. That seems peculiar to me, but I don't really know CTF. In this case you need a comment, since the result will be non-obvious to gdb developers. Tom> check_typedef; though if your intent is to peel just a single layer, Tom> then it is a bit trickier -- I think the best you can do is always call Tom> it, then use TYPE_TARGET_TYPE if it is non-NULL or the result of Tom> check_typedef otherwise. Hui> If use check_typedef, this part will generate the define that Hui> different with the type descriptor of the code. You need to call check_typedef before you can even examine TYPE_TARGET_TYPE of a typedef. This is what I meant by using it before using TYPE_TARGET_TYPE. Otherwise with stubs I think you will see crashes -- check_typedef is what sets this field. If you then use TYPE_TARGET_TYPE and get NULL, you ought to instead use the result of check_typedef. This means the stub had to resolve to a typedef in a different objfile. Hui> If use TYPE_TARGET_TYPE, it will generate following metadata: Hui> typedef char test_t1; Hui> typedef test_t1 test_t2; Hui> typedef test_t2 test_t3; I suppose there should be a test case doing this. Hui> + case TYPE_CODE_PTR: Hui> + align_size = TYPE_LENGTH (type); Hui> + break; Tom> Surely the alignment rules are ABI dependent. Tom> I would guess that what you have will work in many cases, but definitely Tom> not all of them. Hui> All the type will be handle and record in function Hui> ctf_save_type_check_and_write. Hui> The size align will be handle in this function too. I don't think this really addresses the issue. Not all platforms use the alignment rules currently coded in ctf_save_type_check_and_write. But maybe it doesn't matter. Hui> + frame = get_current_frame (); Hui> + if (!frame) Hui> + error (_("get current frame fail")); Hui> + frame = get_prev_frame (frame); Hui> + if (!frame) Hui> + error (_("get prev frame fail")); Tom> Tom> These messages could be improved. Actually, I don't think get_current_frame can return NULL, can it? For the second error, how about "could not find previous frame"? Hui> + warning (_("\ Hui> +Not save \"%s\" of tracepoint %d to ctf file because get its Hui> value fail: %s"), Hui> + str, tps->tp->base.number, e.message); Tom> Tom> Likewise. Hui> Could you help me with this part? :) How about "error saving tracepoint %d to CTF file %s: %s". Tom> Although, this approach just seems weird, since it seems like you Tom> already have the symbol and you want its value; constructing and parsing Tom> an expression to get this is very roundabout. Tom> Tom> I'm not sure I really understand the goal here; but the parsing approach Tom> is particularly fragile if you have shadowing. Hui> Function ctf_save_collect_get will parse the collect string and add Hui> them to struct. Hui> Each tracepoint will call this function just once. Ok, I don't know the answer here. Tom> Hmm, a lot of this code looks like code from tracepoint.c. Tom> I think it would be better to share the code if that is possible. Hui> I tried to share code with function add_local_symbols. But it is not Hui> a big function and use different way to get block. I wonder why, and whether this means that the different ways of saving will in fact write out different data. Hui> + if (collect->expr) Hui> + free_current_contents (&collect->expr); Tom> Why free_current_contents here? Tom> That seems weird. Hui> If this collect is $_ret, it will not have collect->expr. Or maybe Hui> this collect will be free because when setup this collect get Hui> error. So check it before free it. You can just write xfree (collect->expr). You don't need a NULL check here. This applies to all those xfree calls. Tom