DPDK patches and discussions
 help / color / mirror / Atom feed
From: Neil Horman <nhorman@tuxdriver.com>
To: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
Cc: dev@dpdk.org, Bruce Richardson <bruce.richardson@intel.com>,
	Thomas Monjalon <thomas@monjalon.net>,
	robin.jarry@6wind.com
Subject: Re: [dpdk-dev] [RFC PATCH 0/2] pmdinfogen: rewrite in Python
Date: Tue, 23 Jun 2020 07:28:06 -0400
Message-ID: <20200623112806.GA301645@hmswarspite.think-freely.org> (raw)
In-Reply-To: <20200622223946.25977c13@sovereign>

On Mon, Jun 22, 2020 at 10:39:46PM +0300, Dmitry Kozlyuk wrote:
> On Mon, 22 Jun 2020 08:41:17 -0400, Neil Horman wrote:
> > On Mon, Jun 22, 2020 at 03:45:01AM +0300, Dmitry Kozlyuk wrote:
> [snip]
> > > 1. No standard ELF or COFF module for Python
> > >     (amount of Python code without libelf on par with C code using it).  
> > Thats not really true, pyelftools is pretty widely used, and packaged for
> > most(all) major distributions.  Requiring this kicks the can for (2) above down
> > the road a bit, but I would prefer to see that used rather than a custom
> > implementation, just to reduce the maint. burden
> 
> I must have underestimated pyelftools spread. I'll look into using it. The
> less new code the better, I agree here.  Windows users can get it via PyPI.
> 
Ack.

> 
> > > 2. struct rte_pci_id must be synchronized with header file
> > >     (it's a few lines that never change).
> > >   
> > I think you can just use ctypes and the native python C interface to just import
> > the rte_pci_id structure from the native dpdk header to avoid this, no?
> 
> Not sure I follow. RFC script uses ctypes to describe struct rte_pci_id
> in Python. If you're suggesting to create a Python module in C just to include
> a header with a single small structure, I'd say it's not worth the trouble.
> 
Yeah, apologies, i misread what you were saying here, and didn't bother to check
the code.  what you're doing makes sense.

> 
> > > 1. Support for >65K section headers seems present in current pmdinfogen.
> > >     However, the data it reads is not used after. Is it really needed?
> > >   
> > Its used.
> > The support you're referring to is primarily the ability to extract the true
> > number of section headers from the ELF file (in the event that its more than
> > 64k).  Without that, on very large binaries, you may not be able to find the
> > symbol table in the ELF file.
> 
> I was talking about these fields of struct elf_info:
> 
> 	/* if Nth symbol table entry has .st_shndx = SHN_XINDEX,
> 	 * take shndx from symtab_shndx_start[N] instead
> 	 */
> 	Elf32_Word   *symtab_shndx_start;
> 	Elf32_Word   *symtab_shndx_stop;
> 
> They are not used after being filled in parse_elf() and their endianness
> fixed in the end despite the comment.
> 
Its been a while since I wrote this, so I need to go back and look closely, but
as you say, these values aren't used after parse_elf completes, but they are(I
think) used to correct the endianess of the section header index table, so that
get_sym_value works properly.  You would (again I think), only note a problem if
you were parsing an ELF file for an architecture with an endianess opposite that
of what you are building on (i.e. an x86 system cross compiling for a powerpc
target).

> 
> > > 2. How much error-handling is required? This is a build-time tool,
> > >     and Python gives nice stacktraces. However, segfaults are possible
> > >     in Python version due to pointer handling. IMO, error checking
> > >     must be just sufficient to prevent silent segfaults.
> > >   
> > I'm not really sure what the question is here.  You need to handle errors in the
> > parsing process, we can't just have the tool crashing during the build.  How
> > thats handled is an implementation detail I would think.
> 
> Consider a snippet from pmdinfogen.c:
> 
> 	/* Check if file offset is correct */
> 	if (hdr->e_shoff > info->size) {
> 		fprintf(stderr, "section header offset=%lu in file '%s' "
> 		      "is bigger than filesize=%lu\n",
> 		      (unsigned long)hdr->e_shoff,
> 		      filename, info->size);
> 		return 0;
> 	}
> 
> It is required in C, because otherwise pmdinfogen would crash without
> diagnostic. With Python, most errors like this result in a crash with a trace
> to the error line and a message from ctypes (or ELF parsing library). I'm
> arguing for leaving only the checks that prevent *silent* crashes (this can
> happen with ctypes) which are hard to diagnose. Motivation is to keep the
> code simple.
> 
Hmm, I'd defer to others opinions on that.  As a build tool it may be ok to just
crash with a backtrace, but I'm personally not a fan of that.  Id rather see a
diagnostic error and have the tool exits with an appropriate exit code, but lets
see what others say.

> 
> > > 3. On Unix, pmdinfogen is called for each object file extracted with ar
> > >     from an .a library by a shell script. On Windows, other tools
> > >     have to be used, shell script will not work. On the other hand, COFF
> > >     library format is quite simple. Would it be appropriate for
> > >     pmdinfogen to handle it to avoid intermediate script?
> > >   
> > I suppose you could, but extracting objects from archives seems outside the
> > scope of what pmdinfogen normally does.  What is the problem with using a shell
> > script on windows?  I thought WSL had support for executing bash syntax shell
> > scripts there?  Why not key off an environment variable to make the relevant
> > scripts do the right thing on your environment?
> 
> WSL2 is a Linux VM integrated in Windows, you can only cross-compile from
> there. Native builds can use Python or PowerShell, which is as capable as Unix
> shells, but incompatible with them. To call lib.exe (MSVC analog of ar) is
> probably simpler then parsing COFF, yes. So meson could select one of the
> following for a Windows target (objects are COFF):
> 
> Host     Toolchain  Archive  Script      Extractor
> -------  ---------  -------  ---------   ---------
> Linux    MinGW      .a       sh          ar
> Windows  MinGW      .a       PowerShell  ar
> Windows  Clang      .lib     PowerShell  lib
I think if I read you right, what you're suggesting here is that meson setup a
build time marco $ARCHIVETOOL, and set it to ar or lib.exe depending on the
build environment, and just have the script in question use $ARCHIVER?  If so,
I'd be good with that.  Do we have to worry about the use of divergent command
line options for each tool as well?

Neil

> 
> -- 
> Dmitry Kozlyuk
> 

  reply	other threads:[~2020-06-23 11:28 UTC|newest]

Thread overview: 88+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-22  0:45 Dmitry Kozlyuk
2020-06-22  0:45 ` [dpdk-dev] [RFC PATCH 1/2] pmdinfogen: prototype " Dmitry Kozlyuk
2020-06-22  0:45 ` [dpdk-dev] [RFC PATCH 2/2] build: use Python pmdinfogen Dmitry Kozlyuk
2020-06-22 12:41 ` [dpdk-dev] [RFC PATCH 0/2] pmdinfogen: rewrite in Python Neil Horman
2020-06-22 19:39   ` Dmitry Kozlyuk
2020-06-23 11:28     ` Neil Horman [this message]
2020-06-23 11:59       ` Bruce Richardson
2020-07-02  0:07       ` Dmitry Kozlyuk
2020-07-02  0:02 ` [dpdk-dev] [RFC PATCH v2 " Dmitry Kozlyuk
2020-07-02  0:02   ` [dpdk-dev] [RFC PATCH v2 1/3] pmdinfogen: prototype " Dmitry Kozlyuk
2020-07-02  0:02   ` [dpdk-dev] [RFC PATCH v2 2/3] build: use Python pmdinfogen Dmitry Kozlyuk
2020-07-02  0:02   ` [dpdk-dev] [RFC PATCH v2 3/3] doc/linux_gsg: require pyelftools for pmdinfogen Dmitry Kozlyuk
2020-07-06 12:52     ` Neil Horman
2020-07-06 13:24       ` Dmitry Kozlyuk
2020-07-06 16:46         ` Neil Horman
2020-07-08  0:53   ` [dpdk-dev] [PATCH v3 0/4] pmdinfogen: rewrite in Python Dmitry Kozlyuk
2020-07-08  0:53     ` [dpdk-dev] [PATCH v3 1/4] pmdinfogen: add Python implementation Dmitry Kozlyuk
2020-07-08  0:53     ` [dpdk-dev] [PATCH v3 2/4] build: use Python pmdinfogen Dmitry Kozlyuk
2020-07-08  0:53     ` [dpdk-dev] [PATCH v3 3/4] doc/linux_gsg: require pyelftools for pmdinfogen Dmitry Kozlyuk
2020-07-08  0:53     ` [dpdk-dev] [PATCH v3 4/4] pmdinfogen: remove C implementation Dmitry Kozlyuk
2020-07-08 21:23     ` [dpdk-dev] [PATCH v4 0/4] pmdinfogen: rewrite in Python Dmitry Kozlyuk
2020-07-08 21:23       ` [dpdk-dev] [PATCH v4 1/4] pmdinfogen: add Python implementation Dmitry Kozlyuk
2020-07-08 21:23       ` [dpdk-dev] [PATCH v4 2/4] build: use Python pmdinfogen Dmitry Kozlyuk
2020-07-21 14:04         ` Bruce Richardson
2020-07-21 14:59           ` Dmitry Kozlyuk
2020-07-08 21:23       ` [dpdk-dev] [PATCH v4 3/4] doc/linux_gsg: require pyelftools for pmdinfogen Dmitry Kozlyuk
2020-07-21 13:39         ` Bruce Richardson
2020-07-21 14:05           ` Bruce Richardson
2020-07-21 14:04         ` Bruce Richardson
2020-07-08 21:23       ` [dpdk-dev] [PATCH v4 4/4] pmdinfogen: remove C implementation Dmitry Kozlyuk
2020-07-09 10:42       ` [dpdk-dev] [PATCH v4 0/4] pmdinfogen: rewrite in Python Neil Horman
2020-07-21 13:51       ` Bruce Richardson
2020-09-27 21:47       ` [dpdk-dev] [PATCH v5 0/3] " Dmitry Kozlyuk
2020-09-27 21:47         ` [dpdk-dev] [PATCH v5 1/3] pmdinfogen: add Python implementation Dmitry Kozlyuk
2020-09-27 22:05           ` Stephen Hemminger
2020-09-27 21:47         ` [dpdk-dev] [PATCH v5 2/3] build: use Python pmdinfogen Dmitry Kozlyuk
2020-09-27 21:47         ` [dpdk-dev] [PATCH v5 3/3] pmdinfogen: remove C implementation Dmitry Kozlyuk
2020-09-27 23:15           ` Thomas Monjalon
2020-09-28  9:35         ` [dpdk-dev] [PATCH v5 0/3] pmdinfogen: rewrite in Python David Marchand
2020-10-04  1:59         ` [dpdk-dev] [PATCH v6 " Dmitry Kozlyuk
2020-10-04  1:59           ` [dpdk-dev] [PATCH v6 1/3] pmdinfogen: add Python implementation Dmitry Kozlyuk
2020-10-04  1:59           ` [dpdk-dev] [PATCH v6 2/3] build: use Python pmdinfogen Dmitry Kozlyuk
2020-10-04  1:59           ` [dpdk-dev] [PATCH v6 3/3] pmdinfogen: remove C implementation Dmitry Kozlyuk
2020-10-14 14:37           ` [dpdk-dev] [PATCH v6 0/3] pmdinfogen: rewrite in Python Maxime Coquelin
2020-10-14 15:40             ` Dmitry Kozlyuk
2020-10-14 18:31           ` [dpdk-dev] [PATCH v7 " Dmitry Kozlyuk
2020-10-14 18:31             ` [dpdk-dev] [PATCH v7 1/3] pmdinfogen: add Python implementation Dmitry Kozlyuk
2020-10-14 18:31             ` [dpdk-dev] [PATCH v7 2/3] build: use Python pmdinfogen Dmitry Kozlyuk
2020-10-14 18:31             ` [dpdk-dev] [PATCH v7 3/3] pmdinfogen: remove C implementation Dmitry Kozlyuk
2020-10-20 16:02             ` [dpdk-dev] [PATCH v7 0/3] pmdinfogen: rewrite in Python David Marchand
2020-10-20 17:45               ` Dmitry Kozlyuk
2020-10-20 22:09               ` Dmitry Kozlyuk
2020-10-20 17:44             ` [dpdk-dev] [PATCH v8 " Dmitry Kozlyuk
2020-10-20 17:44               ` [dpdk-dev] [PATCH v8 1/3] pmdinfogen: add Python implementation Dmitry Kozlyuk
2020-10-20 17:44               ` [dpdk-dev] [PATCH v8 2/3] build: use Python pmdinfogen Dmitry Kozlyuk
2020-10-21  9:00                 ` Bruce Richardson
2021-01-20  0:05                 ` Thomas Monjalon
2021-01-20  7:23                   ` Dmitry Kozlyuk
2021-01-20 10:24                     ` Thomas Monjalon
2021-01-22 20:31                       ` Dmitry Kozlyuk
2021-01-22 20:57                         ` Thomas Monjalon
2021-01-22 22:24                           ` Dmitry Kozlyuk
2021-01-23 11:38                             ` Thomas Monjalon
2021-01-24 20:52                               ` Dmitry Kozlyuk
2021-01-25  9:25                               ` Kinsella, Ray
2021-01-25 10:01                                 ` Kinsella, Ray
2021-01-25 10:29                                   ` David Marchand
2021-01-25 10:46                                     ` Kinsella, Ray
2021-01-25 11:03                                       ` Thomas Monjalon
2021-01-25 10:05                                 ` Dmitry Kozlyuk
2021-01-25 10:11                                   ` Kinsella, Ray
2021-01-25 10:31                                     ` Dmitry Kozlyuk
2020-10-20 17:44               ` [dpdk-dev] [PATCH v8 3/3] pmdinfogen: remove C implementation Dmitry Kozlyuk
2020-10-26 16:46                 ` Jie Zhou
2021-01-22 22:43               ` [dpdk-dev] [PATCH v9 0/3] pmdinfogen: rewrite in Python Dmitry Kozlyuk
2021-01-22 22:43                 ` [dpdk-dev] [PATCH v9 1/3] pmdinfogen: add Python implementation Dmitry Kozlyuk
2021-01-22 22:43                 ` [dpdk-dev] [PATCH v9 2/3] build: use Python pmdinfogen Dmitry Kozlyuk
2021-01-22 22:43                 ` [dpdk-dev] [PATCH v9 3/3] pmdinfogen: remove C implementation Dmitry Kozlyuk
2021-01-24 20:51                 ` [dpdk-dev] [PATCH v10 0/3] pmdinfogen: rewrite in Python Dmitry Kozlyuk
2021-01-24 20:51                   ` [dpdk-dev] [PATCH v10 1/3] pmdinfogen: add Python implementation Dmitry Kozlyuk
2021-01-24 20:51                   ` [dpdk-dev] [PATCH v10 2/3] build: use Python pmdinfogen Dmitry Kozlyuk
2021-01-25 10:12                     ` Thomas Monjalon
2021-01-24 20:51                   ` [dpdk-dev] [PATCH v10 3/3] pmdinfogen: remove C implementation Dmitry Kozlyuk
2021-01-25 13:13                   ` [dpdk-dev] [PATCH v10 0/3] pmdinfogen: rewrite in Python Thomas Monjalon
2021-01-25 16:08                     ` Brandon Lo
2021-02-02  8:48                       ` Tal Shnaiderman
2021-01-25 18:51                   ` Ali Alnubani
2021-01-25 22:15                     ` Dmitry Kozlyuk

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200623112806.GA301645@hmswarspite.think-freely.org \
    --to=nhorman@tuxdriver.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=dmitry.kozliuk@gmail.com \
    --cc=robin.jarry@6wind.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git