Re: makedumpfile -g with recent kernels

Re: makedumpfile -g with recent kernels

[Atsushi Kumagai]

Hello Stefan,

On Mon, 09 Jul 2012 11:17:13 +0200
Stefan Bader <stefan.bader@canonical.com> wrote:

> Hi,
> 
> I am not sure whether this really is the preferred way as I heard the required
> info now is in vmcore and vmcoreinfo would not be needed anymore. But just to
> have it produce some output I noticed that it trips again over some member of
> struct page that moved into some anonymous structure/union combo.
> There was some code that was quite specific for the page.mapping case (which now
> seems to be outside again). I changed the code to be more generic. It did fix my
> _count problem and should also handle the mapping case (though I could not test).
> So maybe this would be worth adding to the upstream code.
> 
> -Stefan
> 
> Please include me in cc's as I am not subscribed to this ml.

Thank you for your nice work!

I think your idea is basically good, however your patch can't take care of
the member of union (e.g. page.lru in kernel 3.2). 
Because the member of union hasn't DW_AT_data_member_location and dwarf_info.member_offset
isn't be initialized. Therefore, adjust_member_offset() doesn't work for the member of union.

  static void
  adjust_member_offset(Dwarf_Die *die)
  {
          long offset;

          if (dwarf_info.member_offset == NOT_FOUND_STRUCTURE)     // this comparison is always true
                  return;
          if (!get_data_member_location(die, &offset))
                  return;
          dwarf_info.member_offset += offset;
  }

At least, the change below works fine without regression.


diff --git a/dwarf_info.c b/dwarf_info.c
index 583df53..03e4c90 100644
--- a/dwarf_info.c
+++ b/dwarf_info.c
@@ -520,7 +520,9 @@ search_member(Dwarf_Die *die)
                        /*
                         * Get the member offset.
                         */
-                       if (!get_data_member_location(walker, &offset))
+                       if (dwarf_tag(die) == DW_TAG_union_type)
+                               offset = 0;
+                       else if (!get_data_member_location(walker, &offset))
                                continue;
                        dwarf_info.member_offset = offset;
                        return TRUE;


Unless you have better way to fix this issue, I'll merge your patch into
the next version with the change above.

By the way, this fix enable us also to get the offset of page._mapcount and
page.private, it's very helpful for the new method of free page filtering.

  http://lists.infradead.org/pipermail/kexec/2012-June/006441.html


Thanks
Atsushi Kumagai

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: makedumpfile -g with recent kernels

[HATAYAMA Daisuke]

From: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>
Subject: Re: makedumpfile -g with recent kernels
Date: Thu, 2 Aug 2012 14:12:51 +0900

> Hello Stefan,
> 
> On Mon, 09 Jul 2012 11:17:13 +0200
> Stefan Bader <stefan.bader@canonical.com> wrote:
> 
>> Hi,
>> 
>> I am not sure whether this really is the preferred way as I heard the required
>> info now is in vmcore and vmcoreinfo would not be needed anymore. But just to
>> have it produce some output I noticed that it trips again over some member of
>> struct page that moved into some anonymous structure/union combo.
>> There was some code that was quite specific for the page.mapping case (which now
>> seems to be outside again). I changed the code to be more generic. It did fix my
>> _count problem and should also handle the mapping case (though I could not test).
>> So maybe this would be worth adding to the upstream code.
>> 
>> -Stefan
>> 
>> Please include me in cc's as I am not subscribed to this ml.
> 
> Thank you for your nice work!
> 

Hello Kumagai-san,

I want to see a whole part of Stefan's patch. Could you post it on the
ml?

Thanks.
HATAYAMA, Daisuke


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: makedumpfile -g with recent kernels

[Atsushi Kumagai]

Hello HATAYAMA-san,

On Thu, 02 Aug 2012 14:47:14 +0900 (JST)
HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com> wrote:

> From: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>
> Subject: Re: makedumpfile -g with recent kernels
> Date: Thu, 2 Aug 2012 14:12:51 +0900
> 
> > Hello Stefan,
> > 
> > On Mon, 09 Jul 2012 11:17:13 +0200
> > Stefan Bader <stefan.bader@canonical.com> wrote:
> > 
> >> Hi,
> >> 
> >> I am not sure whether this really is the preferred way as I heard the required
> >> info now is in vmcore and vmcoreinfo would not be needed anymore. But just to
> >> have it produce some output I noticed that it trips again over some member of
> >> struct page that moved into some anonymous structure/union combo.
> >> There was some code that was quite specific for the page.mapping case (which now
> >> seems to be outside again). I changed the code to be more generic. It did fix my
> >> _count problem and should also handle the mapping case (though I could not test).
> >> So maybe this would be worth adding to the upstream code.
> >> 
> >> -Stefan
> >> 
> >> Please include me in cc's as I am not subscribed to this ml.
> > 
> > Thank you for your nice work!
> > 
> 
> Hello Kumagai-san,
> 
> I want to see a whole part of Stefan's patch. Could you post it on the
> ml?

This is the Stefan's patch. 


Thanks
Atsushi Kumagai

---
From cde00fffeddddd1fcffd3085ded0947fdcd159ef Mon Sep 17 00:00:00 2001
From: Stefan Bader <stefan.bader@canonical.com>
Date: Thu, 5 Jul 2012 18:03:53 +0200
Subject: [PATCH] Generic search into anonymous members in search_member()

There was a special case to find page.mapping in case it would be
inside an anonymous union.struct. But recent kernel versions also
moved page._count into the depths of some unnamed elements.
Naturally not the same as page.mapping.
So this tries to approach the problem by implementing a generic
method of descending into anonymous sub-members. If this finds
a member and it is the member offset that is the target of the
search, that offset has to be adjusted on the way back out.

Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
---
 dwarf_info.c   |  117 ++++++++++++++++++--------------------------------------
 dwarf_info.h   |    1 -
 makedumpfile.c |    8 ----
 makedumpfile.h |    6 ---
 4 files changed, 38 insertions(+), 94 deletions(-)

diff --git a/dwarf_info.c b/dwarf_info.c
index 1429858..583df53 100644
--- a/dwarf_info.c
+++ b/dwarf_info.c
@@ -64,7 +64,6 @@ is_search_structure(int cmd)
 	if ((cmd == DWARF_INFO_GET_STRUCT_SIZE)
 	    || (cmd == DWARF_INFO_GET_MEMBER_OFFSET)
 	    || (cmd == DWARF_INFO_GET_MEMBER_TYPE)
-	    || (cmd == DWARF_INFO_GET_MEMBER_OFFSET_IN_UNION)
 	    || (cmd == DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION)
 	    || (cmd == DWARF_INFO_GET_MEMBER_ARRAY_LENGTH))
 		return TRUE;
@@ -447,75 +446,40 @@ get_dwarf_base_type(Dwarf_Die *die)
 	return TRUE;
 }
 
-/*
- * Function for searching struct page.union.struct.mapping.
- */
 static int
-__search_mapping(Dwarf_Die *die, long *offset)
+is_anonymous_container(Dwarf_Die *die)
 {
-	int tag;
-	const char *name;
-	Dwarf_Die child, *walker;
-
-	if (dwarf_child(die, &child) != 0)
+	if (dwarf_diename(die))
 		return FALSE;
-
-	walker = &child;
-	do {
-		tag  = dwarf_tag(walker);
-		name = dwarf_diename(walker);
-
-		if (tag != DW_TAG_member)
-			continue;
-		if ((!name) || strcmp(name, dwarf_info.member_name))
-			continue;
-		if (!get_data_member_location(walker, offset))
-			continue;
+	if (dwarf_tag(die) == DW_TAG_union_type)
+		return TRUE;
+	if (dwarf_tag(die) == DW_TAG_structure_type)
 		return TRUE;
-
-	} while (!dwarf_siblingof(walker, walker));
-
 	return FALSE;
 }
 
-/*
- * Function for searching struct page.union.struct.
- */
-static int
-search_mapping(Dwarf_Die *die, long *offset)
+static void
+adjust_member_offset(Dwarf_Die *die)
 {
-	Dwarf_Die child, *walker;
-	Dwarf_Die die_struct;
-
-	if (dwarf_child(die, &child) != 0)
-		return FALSE;
+	long offset;
 
-	walker = &child;
-
-	do {
-		if (dwarf_tag(walker) != DW_TAG_member)
-			continue;
-		if (!get_die_type(walker, &die_struct))
-			continue;
-		if (dwarf_tag(&die_struct) != DW_TAG_structure_type)
-			continue;
-		if (__search_mapping(&die_struct, offset))
-			return TRUE;
-	} while (!dwarf_siblingof(walker, walker));
-
-	return FALSE;
+	if (dwarf_info.member_offset == NOT_FOUND_STRUCTURE)
+		return;
+	if (!get_data_member_location(die, &offset))
+		return;
+	dwarf_info.member_offset += offset;
 }
 
-static void
+static int
 search_member(Dwarf_Die *die)
 {
 	int tag;
-	long offset, offset_union;
+	long offset;
 	const char *name;
-	Dwarf_Die child, *walker, die_union;
+	Dwarf_Die child, *walker, die_type;
 
 	if (dwarf_child(die, &child) != 0)
-		return;
+		return FALSE;
 
 	walker = &child;
 
@@ -526,6 +490,20 @@ search_member(Dwarf_Die *die)
 		if (tag != DW_TAG_member)
 			continue;
 
+		/*
+		 * Descend into anonymous members and search for member
+		 * there.
+		 */
+		if (!name) {
+			if (!get_die_type(walker, &die_type))
+				continue;
+			if (is_anonymous_container(&die_type))
+				if (search_member(&die_type)) {
+					adjust_member_offset(walker);
+					return TRUE;
+				}
+		}
+
 		switch (dwarf_info.cmd) {
 		case DWARF_INFO_GET_MEMBER_TYPE:
 			if ((!name) || strcmp(name, dwarf_info.member_name))
@@ -535,7 +513,7 @@ search_member(Dwarf_Die *die)
 			 */
 			if (!get_dwarf_base_type(walker))
 				continue;
-			return;
+			return TRUE;
 		case DWARF_INFO_GET_MEMBER_OFFSET:
 			if ((!name) || strcmp(name, dwarf_info.member_name))
 				continue;
@@ -545,29 +523,11 @@ search_member(Dwarf_Die *die)
 			if (!get_data_member_location(walker, &offset))
 				continue;
 			dwarf_info.member_offset = offset;
-			return;
-		case DWARF_INFO_GET_MEMBER_OFFSET_IN_UNION:
-			if (!get_die_type(walker, &die_union))
-				continue;
-			if (dwarf_tag(&die_union) != DW_TAG_union_type)
-				continue;
-			/*
-			 * Search page.mapping in union.
-			 */
-			if (!search_mapping(&die_union, &offset_union))
-				continue;
-
-			/*
-			 * Get the member offset.
-			 */
-			if (!get_data_member_location(walker, &offset))
- 				continue;
-			dwarf_info.member_offset = offset + offset_union;
- 			return;
+			return TRUE;
 		case DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION:
-			if (!get_die_type(walker, &die_union))
+			if (!get_die_type(walker, &die_type))
 				continue;
-			if (dwarf_tag(&die_union) != DW_TAG_union_type)
+			if (dwarf_tag(&die_type) != DW_TAG_union_type)
 				continue;
 			/*
 			 * Get the member offset.
@@ -575,7 +535,7 @@ search_member(Dwarf_Die *die)
 			if (!get_data_member_location(walker, &offset))
 				continue;
 			dwarf_info.member_offset = offset;
-			return;
+			return TRUE;
 		case DWARF_INFO_GET_MEMBER_ARRAY_LENGTH:
 			if ((!name) || strcmp(name, dwarf_info.member_name))
 				continue;
@@ -584,14 +544,14 @@ search_member(Dwarf_Die *die)
 			 */
 			if (!get_data_array_length(walker))
 				continue;
-			return;
+			return TRUE;
 		}
 	} while (!dwarf_siblingof(walker, walker));
 
 	/*
 	 * Return even if not found.
 	 */
-	return;
+	return FALSE;
 }
 
 static void
@@ -636,7 +596,6 @@ search_structure(Dwarf_Die *die, int *found)
 		break;
 	case DWARF_INFO_GET_MEMBER_TYPE:
 	case DWARF_INFO_GET_MEMBER_OFFSET:
-	case DWARF_INFO_GET_MEMBER_OFFSET_IN_UNION:
 	case DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION:
 	case DWARF_INFO_GET_MEMBER_ARRAY_LENGTH:
 		search_member(die);
diff --git a/dwarf_info.h b/dwarf_info.h
index 1e07484..8d0084d 100644
--- a/dwarf_info.h
+++ b/dwarf_info.h
@@ -37,7 +37,6 @@
 enum {
 	DWARF_INFO_GET_STRUCT_SIZE,
 	DWARF_INFO_GET_MEMBER_OFFSET,
-	DWARF_INFO_GET_MEMBER_OFFSET_IN_UNION,
 	DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION,
 	DWARF_INFO_GET_MEMBER_ARRAY_LENGTH,
 	DWARF_INFO_GET_SYMBOL_ARRAY_LENGTH,
diff --git a/makedumpfile.c b/makedumpfile.c
index d024e95..d32ce55 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -874,17 +874,9 @@ get_structure_info(void)
 	SIZE_INIT(page, "page");
 	OFFSET_INIT(page.flags, "page", "flags");
 	OFFSET_INIT(page._count, "page", "_count");
-
 	OFFSET_INIT(page.mapping, "page", "mapping");
 ---
 	/*
-	 * On linux-2.6.16 or later, page.mapping is defined
-	 * in anonymous union.
-	 */
-	if (OFFSET(page.mapping) == NOT_FOUND_STRUCTURE)
-		OFFSET_IN_UNION_INIT(page.mapping, "page", "mapping");
-
-	/*
 	 * Some vmlinux(s) don't have debugging information about
 	 * page.mapping. Then, makedumpfile assumes that there is
 	 * "mapping" next to "private(unsigned long)" in the first
diff --git a/makedumpfile.h b/makedumpfile.h
index 6f5489d..4bf502f 100644
--- a/makedumpfile.h
+++ b/makedumpfile.h
@@ -239,12 +239,6 @@ do { \
 	     == FAILED_DWARFINFO) \
 		return FALSE; \
 } while (0)
-#define OFFSET_IN_UNION_INIT(X, Y, Z) \
-do { \
-	if ((OFFSET(X) = get_member_offset(Y, Z, DWARF_INFO_GET_MEMBER_OFFSET_IN_UNION)) \
-	     == FAILED_DWARFINFO) \
-		return FALSE; \
-} while (0)
 #define SYMBOL_ARRAY_LENGTH_INIT(X, Y) \
 do { \
 	if ((ARRAY_LENGTH(X) = get_array_length(Y, NULL, DWARF_INFO_GET_SYMBOL_ARRAY_LENGTH)) == FAILED_DWARFINFO) \
-- 
1.7.9.5

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: makedumpfile -g with recent kernels

[HATAYAMA Daisuke]

From: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>
Subject: Re: makedumpfile -g with recent kernels
Date: Thu, 2 Aug 2012 15:04:29 +0900

> Hello HATAYAMA-san,
> 
> On Thu, 02 Aug 2012 14:47:14 +0900 (JST)
> HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com> wrote:
> 
>> From: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>
>> Subject: Re: makedumpfile -g with recent kernels
>> Date: Thu, 2 Aug 2012 14:12:51 +0900
>> 
>> > Hello Stefan,
>> > 
>> > On Mon, 09 Jul 2012 11:17:13 +0200
>> > Stefan Bader <stefan.bader@canonical.com> wrote:
>> > 
>> >> Hi,
>> >> 
>> >> I am not sure whether this really is the preferred way as I heard the required
>> >> info now is in vmcore and vmcoreinfo would not be needed anymore. But just to
>> >> have it produce some output I noticed that it trips again over some member of
>> >> struct page that moved into some anonymous structure/union combo.
>> >> There was some code that was quite specific for the page.mapping case (which now
>> >> seems to be outside again). I changed the code to be more generic. It did fix my
>> >> _count problem and should also handle the mapping case (though I could not test).
>> >> So maybe this would be worth adding to the upstream code.
>> >> 
>> >> -Stefan
>> >> 
>> >> Please include me in cc's as I am not subscribed to this ml.
>> > 
>> > Thank you for your nice work!
>> > 
>> 
>> Hello Kumagai-san,
>> 
>> I want to see a whole part of Stefan's patch. Could you post it on the
>> ml?
> 
> This is the Stefan's patch. 
> 

Thanks, and this patch helps a lot.

BTW, I think we can get rid of DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION
path, too.

Thanks.
HATAYAMA, Daisuke


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: makedumpfile -g with recent kernels

[Atsushi Kumagai]

Hello HATAYAMA-san,

On Thu, 02 Aug 2012 17:23:05 +0900 (JST)
HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com> wrote:

> From: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>
> Subject: Re: makedumpfile -g with recent kernels
> Date: Thu, 2 Aug 2012 15:04:29 +0900
> 
> > Hello HATAYAMA-san,
> > 
> > On Thu, 02 Aug 2012 14:47:14 +0900 (JST)
> > HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com> wrote:
> > 
> >> From: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>
> >> Subject: Re: makedumpfile -g with recent kernels
> >> Date: Thu, 2 Aug 2012 14:12:51 +0900
> >> 
> >> > Hello Stefan,
> >> > 
> >> > On Mon, 09 Jul 2012 11:17:13 +0200
> >> > Stefan Bader <stefan.bader@canonical.com> wrote:
> >> > 
> >> >> Hi,
> >> >> 
> >> >> I am not sure whether this really is the preferred way as I heard the required
> >> >> info now is in vmcore and vmcoreinfo would not be needed anymore. But just to
> >> >> have it produce some output I noticed that it trips again over some member of
> >> >> struct page that moved into some anonymous structure/union combo.
> >> >> There was some code that was quite specific for the page.mapping case (which now
> >> >> seems to be outside again). I changed the code to be more generic. It did fix my
> >> >> _count problem and should also handle the mapping case (though I could not test).
> >> >> So maybe this would be worth adding to the upstream code.
> >> >> 
> >> >> -Stefan
> >> >> 
> >> >> Please include me in cc's as I am not subscribed to this ml.
> >> > 
> >> > Thank you for your nice work!
> >> > 
> >> 
> >> Hello Kumagai-san,
> >> 
> >> I want to see a whole part of Stefan's patch. Could you post it on the
> >> ml?
> > 
> > This is the Stefan's patch. 
> > 
> 
> Thanks, and this patch helps a lot.
> 
> BTW, I think we can get rid of DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION
> path, too.
> 
> Thanks.
> HATAYAMA, Daisuke

At first, I thought so. But DWARF_INFO_GET_MEMBER_OFFSET doesn't work for
kernel 2.6.16 and 2.6.17 even after applying Stefan's patch.
Additionally, I couldn't find the Die of page.mapping with dwarfdump in that
case. 
I don't know why the Die isn't included in some vmlinuxs, anyway, it seems
that the comment below is true.

    832         /*
    833          * Some vmlinux(s) don't have debugging information about
    834          * page.mapping. Then, makedumpfile assumes that there is
    835          * "mapping" next to "private(unsigned long)" in the first
    836          * union.
    837          */
    838         if (OFFSET(page.mapping) == NOT_FOUND_STRUCTURE) {
    839                 OFFSET(page.mapping) = get_member_offset("page", NULL,
    840                     DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION);

So DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION is still necessary for some kernels.


Thanks
Atsushi Kumagai

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: makedumpfile -g with recent kernels

[HATAYAMA Daisuke]

From: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>
Subject: Re: makedumpfile -g with recent kernels
Date: Tue, 7 Aug 2012 15:24:09 +0900

> Hello HATAYAMA-san,
> 
> On Thu, 02 Aug 2012 17:23:05 +0900 (JST)
> HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com> wrote:
> 
>> From: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>
>> Subject: Re: makedumpfile -g with recent kernels
>> Date: Thu, 2 Aug 2012 15:04:29 +0900
>> 
>> > Hello HATAYAMA-san,
>> > 
>> > On Thu, 02 Aug 2012 14:47:14 +0900 (JST)
>> > HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com> wrote:
>> > 
>> >> From: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>
>> >> Subject: Re: makedumpfile -g with recent kernels
>> >> Date: Thu, 2 Aug 2012 14:12:51 +0900
>> >> 
>> >> > Hello Stefan,
>> >> > 
>> >> > On Mon, 09 Jul 2012 11:17:13 +0200
>> >> > Stefan Bader <stefan.bader@canonical.com> wrote:
>> >> > 
>> >> >> Hi,
>> >> >> 
>> >> >> I am not sure whether this really is the preferred way as I heard the required
>> >> >> info now is in vmcore and vmcoreinfo would not be needed anymore. But just to
>> >> >> have it produce some output I noticed that it trips again over some member of
>> >> >> struct page that moved into some anonymous structure/union combo.
>> >> >> There was some code that was quite specific for the page.mapping case (which now
>> >> >> seems to be outside again). I changed the code to be more generic. It did fix my
>> >> >> _count problem and should also handle the mapping case (though I could not test).
>> >> >> So maybe this would be worth adding to the upstream code.
>> >> >> 
>> >> >> -Stefan
>> >> >> 
>> >> >> Please include me in cc's as I am not subscribed to this ml.
>> >> > 
>> >> > Thank you for your nice work!
>> >> > 
>> >> 
>> >> Hello Kumagai-san,
>> >> 
>> >> I want to see a whole part of Stefan's patch. Could you post it on the
>> >> ml?
>> > 
>> > This is the Stefan's patch. 
>> > 
>> 
>> Thanks, and this patch helps a lot.
>> 
>> BTW, I think we can get rid of DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION
>> path, too.
>> 
>> Thanks.
>> HATAYAMA, Daisuke
> 
> At first, I thought so. But DWARF_INFO_GET_MEMBER_OFFSET doesn't work for
> kernel 2.6.16 and 2.6.17 even after applying Stefan's patch.
> Additionally, I couldn't find the Die of page.mapping with dwarfdump in that
> case. 
> I don't know why the Die isn't included in some vmlinuxs, anyway, it seems
> that the comment below is true.
> 
>     832         /*
>     833          * Some vmlinux(s) don't have debugging information about
>     834          * page.mapping. Then, makedumpfile assumes that there is
>     835          * "mapping" next to "private(unsigned long)" in the first
>     836          * union.
>     837          */
>     838         if (OFFSET(page.mapping) == NOT_FOUND_STRUCTURE) {
>     839                 OFFSET(page.mapping) = get_member_offset("page", NULL,
>     840                     DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION);
> 
> So DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION is still necessary for some kernels.
> 

Thanks for your explanation. I understand it. I have similar
experience where pt_regs sturcture information is missing for some
kernel debuginfo files.

Thanks.
HATAYAMA, Daisuke


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: makedumpfile -g with recent kernels

[Atsushi Kumagai]

Hello Stefan,

On Thu, 02 Aug 2012 10:23:13 +0200
Stefan Bader <stefan.bader@canonical.com> wrote:

> >   static void
> >   adjust_member_offset(Dwarf_Die *die)
> >   {
> >           long offset;
> > 
> >           if (dwarf_info.member_offset == NOT_FOUND_STRUCTURE)     // this comparison is always true
> >                   return;
> >           if (!get_data_member_location(die, &offset))
> >                   return;
> >           dwarf_info.member_offset += offset;
> >   }
> > 
> > At least, the change below works fine without regression.
> > 
> > 
> > diff --git a/dwarf_info.c b/dwarf_info.c
> > index 583df53..03e4c90 100644
> > --- a/dwarf_info.c
> > +++ b/dwarf_info.c
> > @@ -520,7 +520,9 @@ search_member(Dwarf_Die *die)
> >                         /*
> >                          * Get the member offset.
> >                          */
> > -                       if (!get_data_member_location(walker, &offset))
> > +                       if (dwarf_tag(die) == DW_TAG_union_type)
> 
> Hm, should that not be die_type to check what the walker is on? And in that case
> it seems that check was made just before... So maybe it would be ok to assume 0
> as the offset when getting here...?

As you said, the walker will be a member of union or struct when reaching here,
because is_anonymous_container() already checked the walker.

However, get_data_member_location() must succeed if the walker is a member of struct
while it doesn't succeed if the walker is a member of union.
That's why I added the code to check whether the walker is a member of union or not.


Thanks
Atsushi Kumagai

> > +                               offset = 0;
> > +                       else if (!get_data_member_location(walker, &offset))
> >                                 continue;
> >                         dwarf_info.member_offset = offset;
> >                         return TRUE;
> > 
> > 
> > Unless you have better way to fix this issue, I'll merge your patch into
> > the next version with the change above.
> > 
> > By the way, this fix enable us also to get the offset of page._mapcount and
> > page.private, it's very helpful for the new method of free page filtering.
> > 
> >   http://lists.infradead.org/pipermail/kexec/2012-June/006441.html
> > 
> > 
> > Thanks
> > Atsushi Kumagai
> > 
> 
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: makedumpfile -g with recent kernels

[Stefan Bader]

[-- Attachment #1.1: Type: text/plain, Size: 3121 bytes --]

On 06.08.2012 07:54, Atsushi Kumagai wrote:
> Hello Stefan,
> 
> On Thu, 02 Aug 2012 10:23:13 +0200
> Stefan Bader <stefan.bader@canonical.com> wrote:
> 
>>>   static void
>>>   adjust_member_offset(Dwarf_Die *die)
>>>   {
>>>           long offset;
>>>
>>>           if (dwarf_info.member_offset == NOT_FOUND_STRUCTURE)     // this comparison is always true
>>>                   return;
>>>           if (!get_data_member_location(die, &offset))
>>>                   return;
>>>           dwarf_info.member_offset += offset;
>>>   }
>>>
>>> At least, the change below works fine without regression.
>>>
>>>
>>> diff --git a/dwarf_info.c b/dwarf_info.c
>>> index 583df53..03e4c90 100644
>>> --- a/dwarf_info.c
>>> +++ b/dwarf_info.c
>>> @@ -520,7 +520,9 @@ search_member(Dwarf_Die *die)
>>>                         /*
>>>                          * Get the member offset.
>>>                          */
>>> -                       if (!get_data_member_location(walker, &offset))
>>> +                       if (dwarf_tag(die) == DW_TAG_union_type)
>>
>> Hm, should that not be die_type to check what the walker is on? And in that case
>> it seems that check was made just before... So maybe it would be ok to assume 0
>> as the offset when getting here...?
> 
> As you said, the walker will be a member of union or struct when reaching here,
> because is_anonymous_container() already checked the walker.
> 
> However, get_data_member_location() must succeed if the walker is a member of struct
> while it doesn't succeed if the walker is a member of union.
> That's why I added the code to check whether the walker is a member of union or not.

Right, I was just wondering whether that would allow to simplify even more. Like
this:

       case DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION:
                if (!get_die_type(walker, &die_type))
                        continue;
                if (dwarf_tag(&die_type) != DW_TAG_union_type)
                        continue;

                /*
                 * At this point it is clear that this is a union.
                 * Though unions have no offset elements (the offset
                 * is always 0. So get_data_member_location would fail.
                 */
                 dwarf_info.member_offset = 0;
                 return TRUE;

> 
> 
> Thanks
> Atsushi Kumagai
> 
>>> +                               offset = 0;
>>> +                       else if (!get_data_member_location(walker, &offset))
>>>                                 continue;
>>>                         dwarf_info.member_offset = offset;
>>>                         return TRUE;
>>>
>>>
>>> Unless you have better way to fix this issue, I'll merge your patch into
>>> the next version with the change above.
>>>
>>> By the way, this fix enable us also to get the offset of page._mapcount and
>>> page.private, it's very helpful for the new method of free page filtering.
>>>
>>>   http://lists.infradead.org/pipermail/kexec/2012-June/006441.html
>>>
>>>
>>> Thanks
>>> Atsushi Kumagai
>>>
>>
>>



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 900 bytes --]

[-- Attachment #2: Type: text/plain, Size: 143 bytes --]

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: makedumpfile -g with recent kernels

[Atsushi Kumagai]

Hello Stefan,

On Mon, 06 Aug 2012 09:39:53 +0200
Stefan Bader <stefan.bader@canonical.com> wrote:

> On 06.08.2012 07:54, Atsushi Kumagai wrote:
> > Hello Stefan,
> > 
> > On Thu, 02 Aug 2012 10:23:13 +0200
> > Stefan Bader <stefan.bader@canonical.com> wrote:
> > 
> >>>   static void
> >>>   adjust_member_offset(Dwarf_Die *die)
> >>>   {
> >>>           long offset;
> >>>
> >>>           if (dwarf_info.member_offset == NOT_FOUND_STRUCTURE)     // this comparison is always true
> >>>                   return;
> >>>           if (!get_data_member_location(die, &offset))
> >>>                   return;
> >>>           dwarf_info.member_offset += offset;
> >>>   }
> >>>
> >>> At least, the change below works fine without regression.
> >>>
> >>>
> >>> diff --git a/dwarf_info.c b/dwarf_info.c
> >>> index 583df53..03e4c90 100644
> >>> --- a/dwarf_info.c
> >>> +++ b/dwarf_info.c
> >>> @@ -520,7 +520,9 @@ search_member(Dwarf_Die *die)
> >>>                         /*
> >>>                          * Get the member offset.
> >>>                          */
> >>> -                       if (!get_data_member_location(walker, &offset))
> >>> +                       if (dwarf_tag(die) == DW_TAG_union_type)
> >>
> >> Hm, should that not be die_type to check what the walker is on? And in that case
> >> it seems that check was made just before... So maybe it would be ok to assume 0
> >> as the offset when getting here...?
> > 
> > As you said, the walker will be a member of union or struct when reaching here,
> > because is_anonymous_container() already checked the walker.
> > 
> > However, get_data_member_location() must succeed if the walker is a member of struct
> > while it doesn't succeed if the walker is a member of union.
> > That's why I added the code to check whether the walker is a member of union or not.
> 
> Right, I was just wondering whether that would allow to simplify even more. Like
> this:

It seems we misunderstood each other, I talked about DWARF_INFO_GET_MEMBER_OFFSET:

         case DWARF_INFO_GET_MEMBER_OFFSET:
                 if ((!name) || strcmp(name, dwarf_info.member_name))
                         continue;
                 /*
                  * Get the member offset.
                  */
                 if (dwarf_tag(die) == DW_TAG_union_type)
                         offset = 0;
                 else if (!get_data_member_location(walker, &offset))
                         continue;
                 dwarf_info.member_offset = offset;
                 return TRUE;

And about your code:

>        case DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION:
>                 if (!get_die_type(walker, &die_type))
>                         continue;
>                 if (dwarf_tag(&die_type) != DW_TAG_union_type)
>                         continue;
> 
>                 /*
>                  * At this point it is clear that this is a union.
>                  * Though unions have no offset elements (the offset
>                  * is always 0. So get_data_member_location would fail.
>                  */
>                  dwarf_info.member_offset = 0;
>                  return TRUE;

It's wrong that unions have no offset elements.
Correctly, members of union have no offset elements.

So, I'll fix it similarly to DWARF_INFO_GET_MEMBER_OFFSET:

        case DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION:
                if (!get_die_type(walker, &die_type))
                        continue;
                if (dwarf_tag(&die_type) != DW_TAG_union_type)
                        continue;
                /*
                 * Get the member offset.
                 */
                 if (dwarf_tag(die) == DW_TAG_union_type)
                         offset = 0;
                 else if (!get_data_member_location(walker, &offset))
                         continue;
                 dwarf_info.member_offset = offset;
                 return TRUE;

 
Thanks
Atsushi Kumagai

> > 
> > 
> > Thanks
> > Atsushi Kumagai
> > 
> >>> +                               offset = 0;
> >>> +                       else if (!get_data_member_location(walker, &offset))
> >>>                                 continue;
> >>>                         dwarf_info.member_offset = offset;
> >>>                         return TRUE;
> >>>
> >>>
> >>> Unless you have better way to fix this issue, I'll merge your patch into
> >>> the next version with the change above.
> >>>
> >>> By the way, this fix enable us also to get the offset of page._mapcount and
> >>> page.private, it's very helpful for the new method of free page filtering.
> >>>
> >>>   http://lists.infradead.org/pipermail/kexec/2012-June/006441.html
> >>>
> >>>
> >>> Thanks
> >>> Atsushi Kumagai

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: makedumpfile -g with recent kernels

[Atsushi Kumagai]

Hello Stefan,

On Tue, 07 Aug 2012 09:50:49 +0200
Stefan Bader <stefan.bader@canonical.com> wrote:

> > 
> >          case DWARF_INFO_GET_MEMBER_OFFSET:
> >                  if ((!name) || strcmp(name, dwarf_info.member_name))
> >                          continue;
> >                  /*
> >                   * Get the member offset.
> >                   */
> >                  if (dwarf_tag(die) == DW_TAG_union_type)
> >                          offset = 0;
> >                  else if (!get_data_member_location(walker, &offset))
> >                          continue;
> >                  dwarf_info.member_offset = offset;
> >                  return TRUE;
> > 
> > And about your code:
> > 
> >>        case DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION:
> >>                 if (!get_die_type(walker, &die_type))
> >>                         continue;
> >>                 if (dwarf_tag(&die_type) != DW_TAG_union_type)
> >>                         continue;
> >>
> >>                 /*
> >>                  * At this point it is clear that this is a union.
> >>                  * Though unions have no offset elements (the offset
> >>                  * is always 0. So get_data_member_location would fail.
> >>                  */
> >>                  dwarf_info.member_offset = 0;
> >>                  return TRUE;
> > 
> > It's wrong that unions have no offset elements.
> > Correctly, members of union have no offset elements.
> 
> That code was just based on the (incorrect) assumption that we talk about
> *_OFFSET_1ST_UNION and thinking those were the dies that had no offset tags.
> But the members having no offset makes much more sense. But then (without
> looking at dwarf output), if it is only the members of unions that need the
> change (and now I understand why it is checking die and not die_type), the first
> union should always have an offset and that call would only descent into
> structures while looking for the first union and never go past a union.

Did you point out the case like the example below ?
(names of unions are only for explanation.)

Example:
	struct EXAMPLE {                  // top layer
		int A;
		union ONE {               // the target of *_OFFSET_1ST_UNION
			int B;
			union TWO {       // but, my code reaches here
				int C;
			};
		};
	};


Certainly, my change for *_OFFSET_1ST_UNION is wrong.
I changed is_anonymous_container() and attach the fixed patch to this mail.
I'll merge it into the next version unless you have any comment.


Thanks
Atsushi Kumagai

---
 dwarf_info.c   |  122 +++++++++++++++++++-------------------------------------
 dwarf_info.h   |    1 -
 makedumpfile.c |    8 ----
 makedumpfile.h |    6 ---
 4 files changed, 42 insertions(+), 95 deletions(-)

diff --git a/dwarf_info.c b/dwarf_info.c
index 1429858..fb11e49 100644
--- a/dwarf_info.c
+++ b/dwarf_info.c
@@ -64,7 +64,6 @@ is_search_structure(int cmd)
 	if ((cmd == DWARF_INFO_GET_STRUCT_SIZE)
 	    || (cmd == DWARF_INFO_GET_MEMBER_OFFSET)
 	    || (cmd == DWARF_INFO_GET_MEMBER_TYPE)
-	    || (cmd == DWARF_INFO_GET_MEMBER_OFFSET_IN_UNION)
 	    || (cmd == DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION)
 	    || (cmd == DWARF_INFO_GET_MEMBER_ARRAY_LENGTH))
 		return TRUE;
@@ -447,75 +446,41 @@ get_dwarf_base_type(Dwarf_Die *die)
 	return TRUE;
 }
 
-/*
- * Function for searching struct page.union.struct.mapping.
- */
 static int
-__search_mapping(Dwarf_Die *die, long *offset)
+is_anonymous_container(Dwarf_Die *die)
 {
-	int tag;
-	const char *name;
-	Dwarf_Die child, *walker;
-
-	if (dwarf_child(die, &child) != 0)
+	if (dwarf_diename(die))
 		return FALSE;
-
-	walker = &child;
-	do {
-		tag  = dwarf_tag(walker);
-		name = dwarf_diename(walker);
-
-		if (tag != DW_TAG_member)
-			continue;
-		if ((!name) || strcmp(name, dwarf_info.member_name))
-			continue;
-		if (!get_data_member_location(walker, offset))
-			continue;
+	if (dwarf_tag(die) == DW_TAG_structure_type)
+		return TRUE;
+	if (dwarf_info.cmd != DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION
+	    && dwarf_tag(die) == DW_TAG_union_type)
 		return TRUE;
-
-	} while (!dwarf_siblingof(walker, walker));
-
 	return FALSE;
 }
 
-/*
- * Function for searching struct page.union.struct.
- */
-static int
-search_mapping(Dwarf_Die *die, long *offset)
+static void
+adjust_member_offset(Dwarf_Die *die)
 {
-	Dwarf_Die child, *walker;
-	Dwarf_Die die_struct;
-
-	if (dwarf_child(die, &child) != 0)
-		return FALSE;
-
-	walker = &child;
-
-	do {
-		if (dwarf_tag(walker) != DW_TAG_member)
-			continue;
-		if (!get_die_type(walker, &die_struct))
-			continue;
-		if (dwarf_tag(&die_struct) != DW_TAG_structure_type)
-			continue;
-		if (__search_mapping(&die_struct, offset))
-			return TRUE;
-	} while (!dwarf_siblingof(walker, walker));
+	long offset;
 
-	return FALSE;
+	if (dwarf_info.member_offset == NOT_FOUND_STRUCTURE)
+		return;
+	if (!get_data_member_location(die, &offset))
+		return;
+	dwarf_info.member_offset += offset;
 }
 
-static void
+static int
 search_member(Dwarf_Die *die)
 {
 	int tag;
-	long offset, offset_union;
+	long offset;
 	const char *name;
-	Dwarf_Die child, *walker, die_union;
+	Dwarf_Die child, *walker, die_type;
 
 	if (dwarf_child(die, &child) != 0)
-		return;
+		return FALSE;
 
 	walker = &child;
 
@@ -526,6 +491,20 @@ search_member(Dwarf_Die *die)
 		if (tag != DW_TAG_member)
 			continue;
 
+		/*
+		 * Descend into anonymous members and search for member
+		 * there.
+		 */
+		if (!name) {
+			if (!get_die_type(walker, &die_type))
+				continue;
+			if (is_anonymous_container(&die_type))
+				if (search_member(&die_type)) {
+					adjust_member_offset(walker);
+					return TRUE;
+				}
+		}
+
 		switch (dwarf_info.cmd) {
 		case DWARF_INFO_GET_MEMBER_TYPE:
 			if ((!name) || strcmp(name, dwarf_info.member_name))
@@ -535,39 +514,23 @@ search_member(Dwarf_Die *die)
 			 */
 			if (!get_dwarf_base_type(walker))
 				continue;
-			return;
+			return TRUE;
 		case DWARF_INFO_GET_MEMBER_OFFSET:
 			if ((!name) || strcmp(name, dwarf_info.member_name))
 				continue;
 			/*
 			 * Get the member offset.
 			 */
-			if (!get_data_member_location(walker, &offset))
+			if (dwarf_tag(die) == DW_TAG_union_type)
+				offset = 0;
+			else if (!get_data_member_location(walker, &offset))
 				continue;
 			dwarf_info.member_offset = offset;
-			return;
-		case DWARF_INFO_GET_MEMBER_OFFSET_IN_UNION:
-			if (!get_die_type(walker, &die_union))
-				continue;
-			if (dwarf_tag(&die_union) != DW_TAG_union_type)
-				continue;
-			/*
-			 * Search page.mapping in union.
-			 */
-			if (!search_mapping(&die_union, &offset_union))
-				continue;
-
-			/*
-			 * Get the member offset.
-			 */
-			if (!get_data_member_location(walker, &offset))
- 				continue;
-			dwarf_info.member_offset = offset + offset_union;
- 			return;
+			return TRUE;
 		case DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION:
-			if (!get_die_type(walker, &die_union))
+			if (!get_die_type(walker, &die_type))
 				continue;
-			if (dwarf_tag(&die_union) != DW_TAG_union_type)
+			if (dwarf_tag(&die_type) != DW_TAG_union_type)
 				continue;
 			/*
 			 * Get the member offset.
@@ -575,7 +538,7 @@ search_member(Dwarf_Die *die)
 			if (!get_data_member_location(walker, &offset))
 				continue;
 			dwarf_info.member_offset = offset;
-			return;
+			return TRUE;
 		case DWARF_INFO_GET_MEMBER_ARRAY_LENGTH:
 			if ((!name) || strcmp(name, dwarf_info.member_name))
 				continue;
@@ -584,14 +547,14 @@ search_member(Dwarf_Die *die)
 			 */
 			if (!get_data_array_length(walker))
 				continue;
-			return;
+			return TRUE;
 		}
 	} while (!dwarf_siblingof(walker, walker));
 
 	/*
 	 * Return even if not found.
 	 */
-	return;
+	return FALSE;
 }
 
 static void
@@ -636,7 +599,6 @@ search_structure(Dwarf_Die *die, int *found)
 		break;
 	case DWARF_INFO_GET_MEMBER_TYPE:
 	case DWARF_INFO_GET_MEMBER_OFFSET:
-	case DWARF_INFO_GET_MEMBER_OFFSET_IN_UNION:
 	case DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION:
 	case DWARF_INFO_GET_MEMBER_ARRAY_LENGTH:
 		search_member(die);
diff --git a/dwarf_info.h b/dwarf_info.h
index 1e07484..8d0084d 100644
--- a/dwarf_info.h
+++ b/dwarf_info.h
@@ -37,7 +37,6 @@
 enum {
 	DWARF_INFO_GET_STRUCT_SIZE,
 	DWARF_INFO_GET_MEMBER_OFFSET,
-	DWARF_INFO_GET_MEMBER_OFFSET_IN_UNION,
 	DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION,
 	DWARF_INFO_GET_MEMBER_ARRAY_LENGTH,
 	DWARF_INFO_GET_SYMBOL_ARRAY_LENGTH,
diff --git a/makedumpfile.c b/makedumpfile.c
index d024e95..d32ce55 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -874,17 +874,9 @@ get_structure_info(void)
 	SIZE_INIT(page, "page");
 	OFFSET_INIT(page.flags, "page", "flags");
 	OFFSET_INIT(page._count, "page", "_count");
-
 	OFFSET_INIT(page.mapping, "page", "mapping");
 
 	/*
-	 * On linux-2.6.16 or later, page.mapping is defined
-	 * in anonymous union.
-	 */
-	if (OFFSET(page.mapping) == NOT_FOUND_STRUCTURE)
-		OFFSET_IN_UNION_INIT(page.mapping, "page", "mapping");
-
-	/*
 	 * Some vmlinux(s) don't have debugging information about
 	 * page.mapping. Then, makedumpfile assumes that there is
 	 * "mapping" next to "private(unsigned long)" in the first
diff --git a/makedumpfile.h b/makedumpfile.h
index 6f5489d..4bf502f 100644
--- a/makedumpfile.h
+++ b/makedumpfile.h
@@ -239,12 +239,6 @@ do { \
 	     == FAILED_DWARFINFO) \
 		return FALSE; \
 } while (0)
-#define OFFSET_IN_UNION_INIT(X, Y, Z) \
-do { \
-	if ((OFFSET(X) = get_member_offset(Y, Z, DWARF_INFO_GET_MEMBER_OFFSET_IN_UNION)) \
-	     == FAILED_DWARFINFO) \
-		return FALSE; \
-} while (0)
 #define SYMBOL_ARRAY_LENGTH_INIT(X, Y) \
 do { \
 	if ((ARRAY_LENGTH(X) = get_array_length(Y, NULL, DWARF_INFO_GET_SYMBOL_ARRAY_LENGTH)) == FAILED_DWARFINFO) \
-- 
1.7.9.2

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: makedumpfile -g with recent kernels

[Stefan Bader]

[-- Attachment #1.1: Type: text/plain, Size: 10863 bytes --]

On 14.08.2012 07:03, Atsushi Kumagai wrote:
> Hello Stefan,
> 
> On Tue, 07 Aug 2012 09:50:49 +0200
> Stefan Bader <stefan.bader@canonical.com> wrote:
> 
>>>
>>>          case DWARF_INFO_GET_MEMBER_OFFSET:
>>>                  if ((!name) || strcmp(name, dwarf_info.member_name))
>>>                          continue;
>>>                  /*
>>>                   * Get the member offset.
>>>                   */
>>>                  if (dwarf_tag(die) == DW_TAG_union_type)
>>>                          offset = 0;
>>>                  else if (!get_data_member_location(walker, &offset))
>>>                          continue;
>>>                  dwarf_info.member_offset = offset;
>>>                  return TRUE;
>>>
>>> And about your code:
>>>
>>>>        case DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION:
>>>>                 if (!get_die_type(walker, &die_type))
>>>>                         continue;
>>>>                 if (dwarf_tag(&die_type) != DW_TAG_union_type)
>>>>                         continue;
>>>>
>>>>                 /*
>>>>                  * At this point it is clear that this is a union.
>>>>                  * Though unions have no offset elements (the offset
>>>>                  * is always 0. So get_data_member_location would fail.
>>>>                  */
>>>>                  dwarf_info.member_offset = 0;
>>>>                  return TRUE;
>>>
>>> It's wrong that unions have no offset elements.
>>> Correctly, members of union have no offset elements.
>>
>> That code was just based on the (incorrect) assumption that we talk about
>> *_OFFSET_1ST_UNION and thinking those were the dies that had no offset tags.
>> But the members having no offset makes much more sense. But then (without
>> looking at dwarf output), if it is only the members of unions that need the
>> change (and now I understand why it is checking die and not die_type), the first
>> union should always have an offset and that call would only descent into
>> structures while looking for the first union and never go past a union.
> 
> Did you point out the case like the example below ?
> (names of unions are only for explanation.)
> 
> Example:
> 	struct EXAMPLE {                  // top layer
> 		int A;
> 		union ONE {               // the target of *_OFFSET_1ST_UNION
> 			int B;
> 			union TWO {       // but, my code reaches here
> 				int C;
> 			};
> 		};
> 	};
> 
> 
> Certainly, my change for *_OFFSET_1ST_UNION is wrong.
> I changed is_anonymous_container() and attach the fixed patch to this mail.
> I'll merge it into the next version unless you have any comment.

Yes, and that was actually already wrong with my initial patch. Damn these
little details... So now, when looking for the first union, the recursion stops
at the first one. Sounds good.

I think this looks good now... hopefully... :) Thanks.

-Stefan
> 
> 
> Thanks
> Atsushi Kumagai
> 
> ---
>  dwarf_info.c   |  122 +++++++++++++++++++-------------------------------------
>  dwarf_info.h   |    1 -
>  makedumpfile.c |    8 ----
>  makedumpfile.h |    6 ---
>  4 files changed, 42 insertions(+), 95 deletions(-)
> 
> diff --git a/dwarf_info.c b/dwarf_info.c
> index 1429858..fb11e49 100644
> --- a/dwarf_info.c
> +++ b/dwarf_info.c
> @@ -64,7 +64,6 @@ is_search_structure(int cmd)
>  	if ((cmd == DWARF_INFO_GET_STRUCT_SIZE)
>  	    || (cmd == DWARF_INFO_GET_MEMBER_OFFSET)
>  	    || (cmd == DWARF_INFO_GET_MEMBER_TYPE)
> -	    || (cmd == DWARF_INFO_GET_MEMBER_OFFSET_IN_UNION)
>  	    || (cmd == DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION)
>  	    || (cmd == DWARF_INFO_GET_MEMBER_ARRAY_LENGTH))
>  		return TRUE;
> @@ -447,75 +446,41 @@ get_dwarf_base_type(Dwarf_Die *die)
>  	return TRUE;
>  }
>  
> -/*
> - * Function for searching struct page.union.struct.mapping.
> - */
>  static int
> -__search_mapping(Dwarf_Die *die, long *offset)
> +is_anonymous_container(Dwarf_Die *die)
>  {
> -	int tag;
> -	const char *name;
> -	Dwarf_Die child, *walker;
> -
> -	if (dwarf_child(die, &child) != 0)
> +	if (dwarf_diename(die))
>  		return FALSE;
> -
> -	walker = &child;
> -	do {
> -		tag  = dwarf_tag(walker);
> -		name = dwarf_diename(walker);
> -
> -		if (tag != DW_TAG_member)
> -			continue;
> -		if ((!name) || strcmp(name, dwarf_info.member_name))
> -			continue;
> -		if (!get_data_member_location(walker, offset))
> -			continue;
> +	if (dwarf_tag(die) == DW_TAG_structure_type)
> +		return TRUE;
> +	if (dwarf_info.cmd != DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION
> +	    && dwarf_tag(die) == DW_TAG_union_type)
>  		return TRUE;
> -
> -	} while (!dwarf_siblingof(walker, walker));
> -
>  	return FALSE;
>  }
>  
> -/*
> - * Function for searching struct page.union.struct.
> - */
> -static int
> -search_mapping(Dwarf_Die *die, long *offset)
> +static void
> +adjust_member_offset(Dwarf_Die *die)
>  {
> -	Dwarf_Die child, *walker;
> -	Dwarf_Die die_struct;
> -
> -	if (dwarf_child(die, &child) != 0)
> -		return FALSE;
> -
> -	walker = &child;
> -
> -	do {
> -		if (dwarf_tag(walker) != DW_TAG_member)
> -			continue;
> -		if (!get_die_type(walker, &die_struct))
> -			continue;
> -		if (dwarf_tag(&die_struct) != DW_TAG_structure_type)
> -			continue;
> -		if (__search_mapping(&die_struct, offset))
> -			return TRUE;
> -	} while (!dwarf_siblingof(walker, walker));
> +	long offset;
>  
> -	return FALSE;
> +	if (dwarf_info.member_offset == NOT_FOUND_STRUCTURE)
> +		return;
> +	if (!get_data_member_location(die, &offset))
> +		return;
> +	dwarf_info.member_offset += offset;
>  }
>  
> -static void
> +static int
>  search_member(Dwarf_Die *die)
>  {
>  	int tag;
> -	long offset, offset_union;
> +	long offset;
>  	const char *name;
> -	Dwarf_Die child, *walker, die_union;
> +	Dwarf_Die child, *walker, die_type;
>  
>  	if (dwarf_child(die, &child) != 0)
> -		return;
> +		return FALSE;
>  
>  	walker = &child;
>  
> @@ -526,6 +491,20 @@ search_member(Dwarf_Die *die)
>  		if (tag != DW_TAG_member)
>  			continue;
>  
> +		/*
> +		 * Descend into anonymous members and search for member
> +		 * there.
> +		 */
> +		if (!name) {
> +			if (!get_die_type(walker, &die_type))
> +				continue;
> +			if (is_anonymous_container(&die_type))
> +				if (search_member(&die_type)) {
> +					adjust_member_offset(walker);
> +					return TRUE;
> +				}
> +		}
> +
>  		switch (dwarf_info.cmd) {
>  		case DWARF_INFO_GET_MEMBER_TYPE:
>  			if ((!name) || strcmp(name, dwarf_info.member_name))
> @@ -535,39 +514,23 @@ search_member(Dwarf_Die *die)
>  			 */
>  			if (!get_dwarf_base_type(walker))
>  				continue;
> -			return;
> +			return TRUE;
>  		case DWARF_INFO_GET_MEMBER_OFFSET:
>  			if ((!name) || strcmp(name, dwarf_info.member_name))
>  				continue;
>  			/*
>  			 * Get the member offset.
>  			 */
> -			if (!get_data_member_location(walker, &offset))
> +			if (dwarf_tag(die) == DW_TAG_union_type)
> +				offset = 0;
> +			else if (!get_data_member_location(walker, &offset))
>  				continue;
>  			dwarf_info.member_offset = offset;
> -			return;
> -		case DWARF_INFO_GET_MEMBER_OFFSET_IN_UNION:
> -			if (!get_die_type(walker, &die_union))
> -				continue;
> -			if (dwarf_tag(&die_union) != DW_TAG_union_type)
> -				continue;
> -			/*
> -			 * Search page.mapping in union.
> -			 */
> -			if (!search_mapping(&die_union, &offset_union))
> -				continue;
> -
> -			/*
> -			 * Get the member offset.
> -			 */
> -			if (!get_data_member_location(walker, &offset))
> - 				continue;
> -			dwarf_info.member_offset = offset + offset_union;
> - 			return;
> +			return TRUE;
>  		case DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION:
> -			if (!get_die_type(walker, &die_union))
> +			if (!get_die_type(walker, &die_type))
>  				continue;
> -			if (dwarf_tag(&die_union) != DW_TAG_union_type)
> +			if (dwarf_tag(&die_type) != DW_TAG_union_type)
>  				continue;
>  			/*
>  			 * Get the member offset.
> @@ -575,7 +538,7 @@ search_member(Dwarf_Die *die)
>  			if (!get_data_member_location(walker, &offset))
>  				continue;
>  			dwarf_info.member_offset = offset;
> -			return;
> +			return TRUE;
>  		case DWARF_INFO_GET_MEMBER_ARRAY_LENGTH:
>  			if ((!name) || strcmp(name, dwarf_info.member_name))
>  				continue;
> @@ -584,14 +547,14 @@ search_member(Dwarf_Die *die)
>  			 */
>  			if (!get_data_array_length(walker))
>  				continue;
> -			return;
> +			return TRUE;
>  		}
>  	} while (!dwarf_siblingof(walker, walker));
>  
>  	/*
>  	 * Return even if not found.
>  	 */
> -	return;
> +	return FALSE;
>  }
>  
>  static void
> @@ -636,7 +599,6 @@ search_structure(Dwarf_Die *die, int *found)
>  		break;
>  	case DWARF_INFO_GET_MEMBER_TYPE:
>  	case DWARF_INFO_GET_MEMBER_OFFSET:
> -	case DWARF_INFO_GET_MEMBER_OFFSET_IN_UNION:
>  	case DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION:
>  	case DWARF_INFO_GET_MEMBER_ARRAY_LENGTH:
>  		search_member(die);
> diff --git a/dwarf_info.h b/dwarf_info.h
> index 1e07484..8d0084d 100644
> --- a/dwarf_info.h
> +++ b/dwarf_info.h
> @@ -37,7 +37,6 @@
>  enum {
>  	DWARF_INFO_GET_STRUCT_SIZE,
>  	DWARF_INFO_GET_MEMBER_OFFSET,
> -	DWARF_INFO_GET_MEMBER_OFFSET_IN_UNION,
>  	DWARF_INFO_GET_MEMBER_OFFSET_1ST_UNION,
>  	DWARF_INFO_GET_MEMBER_ARRAY_LENGTH,
>  	DWARF_INFO_GET_SYMBOL_ARRAY_LENGTH,
> diff --git a/makedumpfile.c b/makedumpfile.c
> index d024e95..d32ce55 100644
> --- a/makedumpfile.c
> +++ b/makedumpfile.c
> @@ -874,17 +874,9 @@ get_structure_info(void)
>  	SIZE_INIT(page, "page");
>  	OFFSET_INIT(page.flags, "page", "flags");
>  	OFFSET_INIT(page._count, "page", "_count");
> -
>  	OFFSET_INIT(page.mapping, "page", "mapping");
>  
>  	/*
> -	 * On linux-2.6.16 or later, page.mapping is defined
> -	 * in anonymous union.
> -	 */
> -	if (OFFSET(page.mapping) == NOT_FOUND_STRUCTURE)
> -		OFFSET_IN_UNION_INIT(page.mapping, "page", "mapping");
> -
> -	/*
>  	 * Some vmlinux(s) don't have debugging information about
>  	 * page.mapping. Then, makedumpfile assumes that there is
>  	 * "mapping" next to "private(unsigned long)" in the first
> diff --git a/makedumpfile.h b/makedumpfile.h
> index 6f5489d..4bf502f 100644
> --- a/makedumpfile.h
> +++ b/makedumpfile.h
> @@ -239,12 +239,6 @@ do { \
>  	     == FAILED_DWARFINFO) \
>  		return FALSE; \
>  } while (0)
> -#define OFFSET_IN_UNION_INIT(X, Y, Z) \
> -do { \
> -	if ((OFFSET(X) = get_member_offset(Y, Z, DWARF_INFO_GET_MEMBER_OFFSET_IN_UNION)) \
> -	     == FAILED_DWARFINFO) \
> -		return FALSE; \
> -} while (0)
>  #define SYMBOL_ARRAY_LENGTH_INIT(X, Y) \
>  do { \
>  	if ((ARRAY_LENGTH(X) = get_array_length(Y, NULL, DWARF_INFO_GET_SYMBOL_ARRAY_LENGTH)) == FAILED_DWARFINFO) \
> 



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 900 bytes --]

[-- Attachment #2: Type: text/plain, Size: 143 bytes --]

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec