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
prev parent 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.