Bug 242620

Summary: sysutils/lsof: cannot build on current after r354895
Product: Ports & Packages Reporter: Masachika ISHIZUKA <ish>
Component: Individual Port(s)Assignee: Larry Rosenman <ler>
Status: Closed FIXED    
Severity: Affects Some People CC: dougm, dougm, ish, jhb, junchoon, markj, mikael, yasu
Priority: --- Flags: bugzilla: maintainer-feedback? (ler)
Version: Latest   
Hardware: Any   
OS: Any   
Description Flags
use left instead of next in vm_map_entry
use right instead of next in vm_map_entry
patch none

Description Masachika ISHIZUKA 2019-12-13 10:42:44 UTC
After r354895, vm_map_entry has no longer prev/next fields.

# cd /usr/ports/sysutils/lsof
# make
===>  Building for lsof-4.93.2_5,8
--- dproc.o ---
dproc.c:693:23: error: no member named 'next' in 'struct vm_map_entry'
                if (!(ka = (KA_T)e->next))
                                 ~  ^
1 error generated.
*** [dproc.o] Error code 1

make[1]: stopped in /usr/ports/sysutils/lsof/work/lsof-4.93.2
Comment 1 Masachika ISHIZUKA 2019-12-14 00:57:39 UTC
Created attachment 209929 [details]
use left instead of next in vm_map_entry

I don't know we can use left instead of next in vm_map_entry.
Comment 2 Larry Rosenman freebsd_committer 2019-12-14 01:25:23 UTC
@markj, @jhb,  Can either of you help me here?  Or suggest someone else?
Comment 3 Tomoaki AOKI 2019-12-14 05:24:14 UTC
(In reply to Masachika ISHIZUKA from comment #1)
Do you mean "My last OK build was r354895 (r354894?)?"

Element "next" in struct vm_map_entry is integrated with "right" at r355491,
in conjunction with integrating "prev" to "left".

So you should use "right" instead of "left".
Related portion of diff is shown below.

Modified: head/sys/vm/vm_map.h
--- head/sys/vm/vm_map.h	Sat Dec  7 17:10:03 2019	(r355490)
+++ head/sys/vm/vm_map.h	Sat Dec  7 17:14:33 2019	(r355491)
@@ -99,10 +99,8 @@ union vm_map_object {
  *	Also included is control information for virtual copy operations.
 struct vm_map_entry {
-	struct vm_map_entry *prev;	/* previous entry */
-	struct vm_map_entry *next;	/* next entry */
-	struct vm_map_entry *left;	/* left child in binary search tree */
-	struct vm_map_entry *right;	/* right child in binary search tree */
+	struct vm_map_entry *left;	/* left child or previous entry */
+	struct vm_map_entry *right;	/* right child or next entry */
 	vm_offset_t start;		/* start address */
 	vm_offset_t end;		/* end address */
 	vm_offset_t next_read;		/* vaddr of the next sequential read */

It would be better converting to use vm_map_entry_read_succ() introduced at r355538.

Related (non-reverted) commits:

 r355491: https://lists.freebsd.org/pipermail/svn-src-head/2019-December/131611.html

 r355538: https://lists.freebsd.org/pipermail/svn-src-head/2019-December/131642.html
Comment 4 Masachika ISHIZUKA 2019-12-14 08:24:04 UTC
Created attachment 209931 [details]
use right instead of next in vm_map_entry

Thank you Tomoaki AOKI.

> Do you mean "My last OK build was r354895 (r354894?)?"
> Element "next" in struct vm_map_entry is integrated with "right" at r355491,
>in conjunction with integrating "prev" to "left".

I'm sorry, you are correct.

I checked 'https://svnweb.freebsd.org/base/head/sys/vm/vm_map.h?r1=354895&r2=355491' and misstyped previous version number.
Comment 5 Mark Johnston freebsd_committer 2019-12-14 18:33:04 UTC
(In reply to Masachika ISHIZUKA from comment #4)
The patch isn't right, you need to use the vm_map_entry_succ() function now.  Take a look at r355538, which made the same change for libprocstat.
Comment 6 Masachika ISHIZUKA 2019-12-16 02:01:26 UTC
(In reply to Mark Johnston from comment #5)
Thank you for reply.
It is hard to me to create correct patch.
Is there anyone who can create patch ?
Comment 7 Mikael Urankar freebsd_committer 2019-12-17 14:21:51 UTC
Created attachment 210006 [details]

Not sure I got this right, at least it doesn't segfault. It should only be applied on -current.
Comment 8 Mark Johnston freebsd_committer 2019-12-17 15:18:01 UTC
(In reply to mikael.urankar from comment #7)
This is a step in the right direction, but not quite right: kvm_read() returns the number of bytes read, and -1 upon an error.  The reader function should compare the return value of kvm_read() with sizeof(*dest).  The function should probably also be named something else, since it has nothing to do with procstat.  vm_map_reader() is probably fine.
Comment 9 Doug Moore 2019-12-17 18:00:01 UTC
(In reply to Mark Johnston from comment #8)
vm_map_entry_read_succ modifies the vm_map_entry struct that its second argument points to, by (generally) kvm-reading a new value into it.  So this patch appears to modify vmsp.vm_map_header on iteration i==1.

I suggest something like

vmme = vmsp.vm_map_header;
e = &vmme;

to get started.
Comment 10 Larry Rosenman freebsd_committer 2019-12-17 18:42:28 UTC
I would *REALLY* love some support from the kernel folks in getting lsof up to snuff with the current changes.  

I'm not a great C programmer, and the changes are significant.

THere's also a great opportunity now to clean up all the way old cruft, but I'm *NOT* the person to do that. 

Comment 11 commit-hook freebsd_committer 2019-12-21 02:02:01 UTC
A commit references this bug:

Author: ler
Date: Sat Dec 21 02:01:52 UTC 2019
New revision: 520539
URL: https://svnweb.freebsd.org/changeset/ports/520539

  sysutils/lsof: unbreak after base r354895.

  - fix vm_map related breakage
  - remove #define/#undef _KERNEL for tmpfs (dougm had the header fixed)

  patch courtesy of dougm@

  PR:		242620
  Reported by:	ish@amail.plala.or.jp
  Obtained from:	dougm

Comment 12 Larry Rosenman freebsd_committer 2019-12-21 02:03:05 UTC
dougm@ gave me a patch that I just committed.