Re: [PATCH]Add symbol whose field 'has_type' has been set to partial symbol table

Re: [PATCH]Add symbol whose field 'has_type' has been set to partial symbol table

[hex]

[-- Attachment #1: Type: text/plain, Size: 1040 bytes --]

> 2013/10/30 Tom Tromey <tromey@redhat.com>
>
> >>>>> ">" == hex  <heixia108@gmail.com> writes:
>
> >> Thank you for the review. I have attached the test case.
>
> The test case needs a ChangeLog entry.
>
> I'd much prefer a test using the DWARF assembler than one using actual
> assembly code.  There are other examples in gdb.dwarf2.  The reason to
> prefer the DWARF assembler is that it is more portable.
>
> >> # You should have received a copy of the GNU General Public License
> >> # along with this program.  If not, see <http://www.gnu.org/licenses/>.
> >> load_lib dwarf.exp
>
> A blank line between the comments and the first line of code, please.
>
> >> # Test DW_OP_stack_value and DW_OP_implicit_value.
>
> This comment seems incorrect.
>
> Tom

Thank you for the comments. A new version of test file is attached.
And the original patch is at
https://sourceware.org/ml/gdb-patches/2013-10/msg00747.html.

gdb/testsuite/Changelog:

2013-11-01  Jun Gong  <heixia108@gmail.com>

      * gdb.dwarf2/const-var.exp: New file.


Jun

[-- Attachment #2: const-var.exp --]
[-- Type: application/octet-stream, Size: 1749 bytes --]

# Copyright 2013 Free Software Foundation, Inc.

# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation; either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program.  If not, see <http://www.gnu.org/licenses/>.

load_lib dwarf.exp

# This test can only be run on targets which support DWARF-2 and use gas.
if {![dwarf2_support]} {
    return 0
}

standard_testfile main.c .S

# Create the DWARF.
set asm_file [standard_output_file $srcfile2]
Dwarf::assemble $asm_file {
    declare_labels int_label

    cu {} {
	compile_unit {{language @DW_LANG_C}} {
	    int_label: base_type {
		{name int}
		{byte_size 4 sdata}
		{encoding @DW_ATE_signed}
	    }

	    DW_TAG_variable {
		{name const_var}
		{type :$int_label}
		{const_value 6 data1}
	    }
	}
    }
}

if  { [gdb_compile ${srcdir}/${subdir}/${srcfile} ${binfile}1.o \
	   object {nodebug}] != "" } {
    return -1
}

if  { [gdb_compile $asm_file ${binfile}2.o object {nodebug}] != "" } {
    return -1
}

if  { [gdb_compile [list ${binfile}1.o ${binfile}2.o] \
	   "${binfile}" executable {}] != "" } {
    return -1
}

clean_restart ${testfile}

if ![runto_main] {
    return -1
}

# Print const variable, test whether it has been added to partial
# symbol table.
gdb_test "print const_var" " = 6" "print const variable"

Re: [PATCH]Add symbol whose field 'has_type' has been set to partial symbol table

[Tom Tromey]

>>>>> ">" == hex  <heixia108@gmail.com> writes:

>> 2013-11-01  Jun Gong  <heixia108@gmail.com>
>>       * gdb.dwarf2/const-var.exp: New file.

Thanks, it all looks good now.

Do you have a copyright assignment on file?
If not, please contact me off-list and I will get you started on the
process.

Tom

[PATCH]Add symbol whose field 'has_type' has been set to partial symbol table

[Jun Gong]

[-- Attachment #1: Type: text/plain, Size: 1645 bytes --]

This patch intends to add symbol whose field 'has_type' has been set
to partial symbol table.

The patch and corresponding test case have been discussed and reviewed
before at https://sourceware.org/ml/gdb-patches/2013-10/msg00747.html
and https://sourceware.org/ml/gdb-patches/2013-11/msg00003.html.
However it has not been committed because Tom Tromey is very busy and
could not help commit it now. I have received copyright assignment.
Could you help review and commit the patch? Thank you!

The following are the summary of the patch, and the attachment
const-var.exp is test case.

1. Change for gdb/ChangeLog

2014-01-24  Jun Gong  <heixia108@gmail.com>

          * dwarf2read.c(add_partial_symbol): Add symbol whose field
'has_type' has been set to partial symbol table.

2. Diff for dwarf2read.c

diff -up gdb-7.7.50.20140124.orig/gdb/dwarf2read.c
gdb-7.7.50.20140124/gdb/dwarf2read.c
--- gdb-7.7.50.20140124.orig/gdb/dwarf2read.c 2014-01-24
13:04:04.169111566 +0800
+++ gdb-7.7.50.20140124/gdb/dwarf2read.c 2014-01-24 13:26:48.445122774 +0800
@@ -6769,8 +6769,9 @@ add_partial_symbol (struct partial_die_i
  }
       else
  {
-  /* Static Variable.  Skip symbols without location descriptors.  */
-  if (pdi->d.locdesc == NULL)
+  /* Static Variable.  Skip symbols without location descriptors. or
+      has_type not set. */
+  if (pdi->d.locdesc == NULL && pdi->has_type == 0)
     {
       xfree (built_actual_name);
       return;

3. Change for gdb/testsuite/Changelog

2014-01-24  Jun Gong  <heixia108@gmail.com>

      * gdb.dwarf2/const-var.exp: New file.

4. Test case has been attached as const-var.exp.


Best Regards,

Jun

[-- Attachment #2: const-var.exp --]
[-- Type: application/octet-stream, Size: 1749 bytes --]

# Copyright 2013 Free Software Foundation, Inc.

# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation; either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program.  If not, see <http://www.gnu.org/licenses/>.

load_lib dwarf.exp

# This test can only be run on targets which support DWARF-2 and use gas.
if {![dwarf2_support]} {
    return 0
}

standard_testfile main.c .S

# Create the DWARF.
set asm_file [standard_output_file $srcfile2]
Dwarf::assemble $asm_file {
    declare_labels int_label

    cu {} {
	compile_unit {{language @DW_LANG_C}} {
	    int_label: base_type {
		{name int}
		{byte_size 4 sdata}
		{encoding @DW_ATE_signed}
	    }

	    DW_TAG_variable {
		{name const_var}
		{type :$int_label}
		{const_value 6 data1}
	    }
	}
    }
}

if  { [gdb_compile ${srcdir}/${subdir}/${srcfile} ${binfile}1.o \
	   object {nodebug}] != "" } {
    return -1
}

if  { [gdb_compile $asm_file ${binfile}2.o object {nodebug}] != "" } {
    return -1
}

if  { [gdb_compile [list ${binfile}1.o ${binfile}2.o] \
	   "${binfile}" executable {}] != "" } {
    return -1
}

clean_restart ${testfile}

if ![runto_main] {
    return -1
}

# Print const variable, test whether it has been added to partial
# symbol table.
gdb_test "print const_var" " = 6" "print const variable"

Re: [PATCH]Add symbol whose field 'has_type' has been set to partial symbol table

[Yao Qi]

On 01/24/2014 02:19 PM, Jun Gong wrote:
> This patch intends to add symbol whose field 'has_type' has been set
> to partial symbol table.
> 
> The patch and corresponding test case have been discussed and reviewed
> before at https://sourceware.org/ml/gdb-patches/2013-10/msg00747.html
> and https://sourceware.org/ml/gdb-patches/2013-11/msg00003.html.
> However it has not been committed because Tom Tromey is very busy and
> could not help commit it now. I have received copyright assignment.
> Could you help review and commit the patch? Thank you!

Thanks for submitting this patch.  Patch itself looks simple, but
doesn't meet some requirements for patch submission.  In short,
please have a look at https://sourceware.org/gdb/wiki/ContributionChecklist

> 
> The following are the summary of the patch, and the attachment
> const-var.exp is test case.
> 
> 1. Change for gdb/ChangeLog
> 
> 2014-01-24  Jun Gong  <heixia108@gmail.com>
> 
>           * dwarf2read.c(add_partial_symbol): Add symbol whose field

Each entry should be tab-prefixed.  A space is missing before "(".

> 'has_type' has been set to partial symbol table.

Should be tab-prefixed.

> 
> 2. Diff for dwarf2read.c
> 
> diff -up gdb-7.7.50.20140124.orig/gdb/dwarf2read.c
> gdb-7.7.50.20140124/gdb/dwarf2read.c
> --- gdb-7.7.50.20140124.orig/gdb/dwarf2read.c 2014-01-24
> 13:04:04.169111566 +0800
> +++ gdb-7.7.50.20140124/gdb/dwarf2read.c 2014-01-24 13:26:48.445122774 +0800
> @@ -6769,8 +6769,9 @@ add_partial_symbol (struct partial_die_i
>   }
>        else
>   {
> -  /* Static Variable.  Skip symbols without location descriptors.  */
> -  if (pdi->d.locdesc == NULL)
> +  /* Static Variable.  Skip symbols without location descriptors. or
> +      has_type not set. */

The period after "descriptors" should be removed.  It is better to say

"Skip symbols without location descriptors or don't have type", or
whatever "pdi->has_type == 0" stands for.

> +  if (pdi->d.locdesc == NULL && pdi->has_type == 0)

pdi->has_type is a boolean to me, we can use "!pdi->has_type".

>      {
>        xfree (built_actual_name);
>        return;
> 
> 3. Change for gdb/testsuite/Changelog
> 
> 2014-01-24  Jun Gong  <heixia108@gmail.com>
> 
>       * gdb.dwarf2/const-var.exp: New file.
> 
> 4. Test case has been attached as const-var.exp.
> 

Attaching a new file gets the review a little hard.  Please generate a
diff including your changes above and this new file.  That is easier
for people to review.  Write the changelog in the e-mail together,

gdb:

2014-01-24  Jun Gong  <heixia108@gmail.com>

	* dwarf2read.c(add_partial_symbol): Add symbol whose field
	'has_type' has been set to partial symbol table.

gdb/testsuite:

2014-01-24  Jun Gong  <heixia108@gmail.com>

      * gdb.dwarf2/const-var.exp: New file.

and attach the patch including the changes in dwarf2read.c and new test
cases in the e-mail.

-- 
Yao (齐尧)

[PATCH]Add symbol whose field 'has_type' has been set to partial symbol table

[hex]

This patch intends to add symbol whose field 'has_type' has been set
to partial symbol table.

e.g.
// test.c
const int var = 3;

// Then compile it with `gcc -g -c test.c`(For the lastest GCC, we
need use 'gcc -g -O1 -c test.' to get the following DIE)
We could see the DIE of 'var' is as following:
 <1><25>: Abbrev Number: 2 (DW_TAG_variable)
    <26>   DW_AT_name        : var
    <2a>   DW_AT_decl_file   : 1
    <2b>   DW_AT_decl_line   : 1
    <2c>   DW_AT_type        : <0x31>
    <30>   DW_AT_const_value : 3

Latest GDB will not add it to partial symbol table because its symbol
satisfies 'pdi->d.locdesc == NULL'.  I think we need add it to partial
symbol table.


2013-10-24  Jun Gong  <heixia108@gmail.com>

          * dwarf2read.c(add_partial_symbol): Add symbol whose field
'has_type' has been set to partial symbol table.


--- gdb-7.6.50.20131021.orig/gdb/dwarf2read.c 2013-10-16
10:55:27.000000000 +0800
+++ gdb-7.6.50.20131021/gdb/dwarf2read.c 2013-10-24 14:29:18.737067814 +0800
@@ -6733,8 +6733,9 @@ add_partial_symbol (struct partial_die_i
  }
       else
  {
-  /* Static Variable.  Skip symbols without location descriptors.  */
-  if (pdi->d.locdesc == NULL)
+  /* Static Variable.  Skip symbols without location descriptors or
+     has_type not set.  */
+  if (pdi->d.locdesc == NULL && pdi->has_type == 0)
     {
       xfree (built_actual_name);
       return;

Re: [PATCH]Add symbol whose field 'has_type' has been set to partial symbol table

[Tom Tromey]

>>>>> ">" == hex  <heixia108@gmail.com> writes:

>> // Then compile it with `gcc -g -c test.c`(For the lastest GCC, we
>> need use 'gcc -g -O1 -c test.' to get the following DIE)
>> We could see the DIE of 'var' is as following:
>>  <1><25>: Abbrev Number: 2 (DW_TAG_variable)
>>     <26>   DW_AT_name        : var
>>     <2a>   DW_AT_decl_file   : 1
>>     <2b>   DW_AT_decl_line   : 1
>>     <2c>   DW_AT_type        : <0x31>
>>     <30>   DW_AT_const_value : 3

>> Latest GDB will not add it to partial symbol table because its symbol
>> satisfies 'pdi->d.locdesc == NULL'.  I think we need add it to partial
>> symbol table.

This patch seems reasonable to me, but I think it needs a test case.
This should be easy to write using the DWARF assembler in the test suite.

Tom

Re: [PATCH]Add symbol whose field 'has_type' has been set to partial symbol table

[hex]

[-- Attachment #1: Type: text/plain, Size: 907 bytes --]

2013/10/25 Tom Tromey <tromey@redhat.com>:
>>>>>> ">" == hex  <heixia108@gmail.com> writes:
>
>>> // Then compile it with `gcc -g -c test.c`(For the lastest GCC, we
>>> need use 'gcc -g -O1 -c test.' to get the following DIE)
>>> We could see the DIE of 'var' is as following:
>>>  <1><25>: Abbrev Number: 2 (DW_TAG_variable)
>>>     <26>   DW_AT_name        : var
>>>     <2a>   DW_AT_decl_file   : 1
>>>     <2b>   DW_AT_decl_line   : 1
>>>     <2c>   DW_AT_type        : <0x31>
>>>     <30>   DW_AT_const_value : 3
>
>>> Latest GDB will not add it to partial symbol table because its symbol
>>> satisfies 'pdi->d.locdesc == NULL'.  I think we need add it to partial
>>> symbol table.
>
> This patch seems reasonable to me, but I think it needs a test case.
> This should be easy to write using the DWARF assembler in the test suite.
>
> Tom

Thank you for the review. I have attached the test case.

Jun

[-- Attachment #2: const-var.S --]
[-- Type: text/plain, Size: 2711 bytes --]

/*
   Copyright 2009-2013 Free Software Foundation, Inc.

   This program is free software; you can redistribute it and/or modify
   it under the terms of the GNU General Public License as published by
   the Free Software Foundation; either version 3 of the License, or
   (at your option) any later version.

   This program is distributed in the hope that it will be useful,
   but WITHOUT ANY WARRANTY; without even the implied warranty of
   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
   GNU General Public License for more details.

   You should have received a copy of the GNU General Public License
   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */

/* This was compiled from a trivial program just to test whether GDB
   will add const variable to partial symbol.

  static const int const_var = 6;

  int main(){
          return 0;
  }

  Then it was compiled with:
	
	gcc -g -O1
  
  We could see the DIE of 'const_var' is as following:
  <1><25>: Abbrev Number: 2 (DW_TAG_variable)
     <26>   DW_AT_name        : var
     <2a>   DW_AT_decl_file   : 1
     <2b>   DW_AT_decl_line   : 1
     <2c>   DW_AT_type        : <0x31>
     <30>   DW_AT_const_value : 3

  We need add it to partial symbol table.

*/

	.file	"const-var.c"
	.text
.Ltext0:
.Letext0:
	.file 1 "/tmp/const-var.c"
	.section	.debug_info,"",@progbits
.Ldebug_info0:
	.long	0x36
	.value	0x2
	.long	.Ldebug_abbrev0
	.byte	0x4
	.uleb128 0x1
	.long	.LASF0
	.byte	0x1
	.long	.LASF1
	.long	.Ltext0
	.long	.Letext0
	.long	.Ldebug_line0
	.uleb128 0x2
	.long	.LASF2
	.byte	0x1
	.byte	0x1
	.long	0x2d
	.byte	0x6
	.uleb128 0x3
	.long	0x32
	.uleb128 0x4
	.byte	0x4
	.byte	0x5
	.string	"int"
	.byte	0
	.section	.debug_abbrev,"",@progbits
.Ldebug_abbrev0:
	.uleb128 0x1
	.uleb128 0x11
	.byte	0x1
	.uleb128 0x25
	.uleb128 0xe
	.uleb128 0x13
	.uleb128 0xb
	.uleb128 0x3
	.uleb128 0xe
	.uleb128 0x11
	.uleb128 0x1
	.uleb128 0x12
	.uleb128 0x1
	.uleb128 0x10
	.uleb128 0x6
	.byte	0
	.byte	0
	.uleb128 0x2
	.uleb128 0x34
	.byte	0
	.uleb128 0x3
	.uleb128 0xe
	.uleb128 0x3a
	.uleb128 0xb
	.uleb128 0x3b
	.uleb128 0xb
	.uleb128 0x49
	.uleb128 0x13
	.uleb128 0x1c
	.uleb128 0xb
	.byte	0
	.byte	0
	.uleb128 0x3
	.uleb128 0x26
	.byte	0
	.uleb128 0x49
	.uleb128 0x13
	.byte	0
	.byte	0
	.uleb128 0x4
	.uleb128 0x24
	.byte	0
	.uleb128 0xb
	.uleb128 0xb
	.uleb128 0x3e
	.uleb128 0xb
	.uleb128 0x3
	.uleb128 0x8
	.byte	0
	.byte	0
	.byte	0
	.section	.debug_line,"",@progbits
.Ldebug_line0:
	.section	.debug_str,"MS",@progbits,1
.LASF0:
	.string	"GNU C 4.6.3"
.LASF1:
	.string	"/tmp/const-var.c"
.LASF2:
	.string	"const_var"
	.ident	"GCC: (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3"
	.section	.note.GNU-stack,"",@progbits

[-- Attachment #3: const-var.exp --]
[-- Type: application/octet-stream, Size: 1296 bytes --]

# Copyright 2009-2013 Free Software Foundation, Inc.

# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation; either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program.  If not, see <http://www.gnu.org/licenses/>.
load_lib dwarf.exp

# Test DW_OP_stack_value and DW_OP_implicit_value.

# This test can only be run on targets which support DWARF-2 and use gas.
if {![dwarf2_support]} {
    return 0  
}
# This test can only be run on x86 targets.
if {![is_x86_like_target]} {
    return 0  
}

standard_testfile .S

if {[prepare_for_testing $testfile.exp $testfile \
    [list $srcfile main.c] [list {additional_flags=-O1}]]} {
    return -1
}

if ![runto_main] {
    return -1
}

# Print static const variable, test whether it has been added to partial
# symbol table.
gdb_test "p const_var" " = 6" "print const variable"

Re: [PATCH]Add symbol whose field 'has_type' has been set to partial symbol table

[Tom Tromey]

>>>>> ">" == hex  <heixia108@gmail.com> writes:

>> Thank you for the review. I have attached the test case.

The test case needs a ChangeLog entry.

I'd much prefer a test using the DWARF assembler than one using actual
assembly code.  There are other examples in gdb.dwarf2.  The reason to
prefer the DWARF assembler is that it is more portable.

>> # You should have received a copy of the GNU General Public License
>> # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> load_lib dwarf.exp

A blank line between the comments and the first line of code, please.

>> # Test DW_OP_stack_value and DW_OP_implicit_value.

This comment seems incorrect.

Tom