All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ingo Molnar <mingo@elte.hu>
To: Jan Beulich <jbeulich@novell.com>
Cc: Alexander van Heukelum <heukelum@fastmail.fm>,
	Cyrill Gorcunov <gorcunov@gmail.com>,
	tglx@linutronix.de, linux-kernel@vger.kernel.org, hpa@zytor.com,
	Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH] x86-64: fix unwind annotations in entry_64.S
Date: Thu, 12 Mar 2009 12:55:01 +0100	[thread overview]
Message-ID: <20090312115501.GA10391@elte.hu> (raw)
In-Reply-To: <49B8FE3C.76E4.0078.0@novell.com>


* Jan Beulich <jbeulich@novell.com> wrote:

> > The current annotations might be completely broken but at 
> > least they _look_ structured and attempt to bring some order 
> > into the CFI annotations chaos.
> 
> There was no chaos. Everything was working fine (and was 
> understandable to someone familiar with CFI annotations). 
> [...]

Well, in the patch you remove a ton of incorrect annotations, 
which all came from the old code. So the old code was all but 
fine - it was ugly and kept breaking all the time.

The thing is, we were this -->.<-- close to removing _all_ CFI 
annotations from x86 assembly files about a year ago. I actually 
wrote all the patches for that and committed them back then.

The reason: they made the assembly code even harder to read and 
distracted from the primary, functional purpose of the code - 
which is functionality. Writing assembly code is hard enough, we 
dont need hundreds of additional CFI annotations to obscure them 
as well.

So when you add back CFI annotations this should be your main 
driving principle: make them as unintrusive as possible. Merge 
them to existing frame layout macros wherever possible. Dont add 
raw CFI annotations. You dont seem to understand and respect any 
of these principles.

You seem to regard them as on par with code but they are not. 
They are at most an afterthought and we will not tolerate them 
if they look ugly - and heck does your patch look ugly.

So we want to hide the CFI details as much as possible 
technically, and make it all as self-maintaining as possible.

Document each and every type of annotation and perhaps also 
write up a small blurb in one of the include files about the 
rules and practices needed for good CFI annotations. That way 
you teach others how to keep it all working fine. Perhaps think 
about writing a KGDB drivn self-test that finds CFI annotation 
bugs - _anything_ that brings this code out of obscurity.

I dont mind quirky features that external entities like your 
dwarf2 unwinder rely on - as long as they benefit the code and 
as long as you participate in the process and as long a you are 
willing to structure it all in a clean way. Coming out of the 
corporate woodwork every 6 months and complaining that "it is 
broken currently" and then going back into hiding without 
changing the dynamics of the code is not enough.

	Ingo

      reply	other threads:[~2009-03-12 11:56 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-03-12 10:32 [PATCH] x86-64: fix unwind annotations in entry_64.S Jan Beulich
2009-03-12 10:48 ` Ingo Molnar
2009-03-12 11:21   ` Jan Beulich
2009-03-12 11:55     ` Ingo Molnar [this message]

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=20090312115501.GA10391@elte.hu \
    --to=mingo@elte.hu \
    --cc=gorcunov@gmail.com \
    --cc=heukelum@fastmail.fm \
    --cc=hpa@zytor.com \
    --cc=jbeulich@novell.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.