This is the mail archive of the cygwin-apps mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch/rebase] Add a rebase database to keep track of DLL addresses


On Jul  6 11:27, Charles Wilson wrote:
> On 7/6/2011 4:41 AM, Corinna Vinschen wrote:
> > On Jul  5 15:38, Charles Wilson wrote:
> >> img_info_cmp: what if a or b is NULL? do we care?  Ditto for a->name,
> >> b->name -- POSIX doesn't mandate the behavior of strcmp if args are null...
> > 
> > Neither the a/b pointers, nor the name strings can become NULL.
> 
> So, the "protection" is algorithmic -- you simply ensure that this
> function is not used in cases where those pointers are null.

If the pointers can get NULL, it's a bug in the calling function.

> (And there ARE cases where they CAN be null: e.g. the newly-allocated
> entries at the tail of the list -- since you allocate in rounded-up
> chunks of 100(?); the name pointers are, of course, explicitly SET to
> null when reading data in from the db, prior to reading and allocating
> storage for the strings themselves, etc).

Oh, come on.  These entries are never used in bsearch or qsort.  If
these empty entries are ever used, it's again a bug in the calling
function.  You just don't call qsort or bsearch on uninitialized data.
So what's the point?

> The main reasons for this rewrite are, that
> 1. WinMe does not support the RebaseImage()
> 2. the recent binutils strip command corrupts dll's linked with
> debugging informations.
> 
> OK, so #1 doesn't apply anymore (technically msys and MinGW still kinda
> maybe support Win9x [*] in the "if you're lucky it'll work but don't
> come crying to us if it doesn't").  I'm not really sure what the story
> is behind #2...

This is very old stuff.

> Anyway, now imagehelper uses cygwin pathname conversion functions and
> the like, so it does a bit more than simply re-implementing ReBaseImage.

Right, but there wouldn't be a problem to do the path conversion before
calling ReBaseImage64.

> > Without this decision first, I didn't want to clutter the database
> > file format with stuff it doesn't need.
> 
> OK.
> 
> > What's more, the rebase addresses of 32 and 64 bit DLLs have nothing to
> > do with each other.  Since they never share the same VM space, they are
> > disjunct.  I mulled over a combined database format for both versions,
> > but it doesn't make sense.  I think, what we should do is to keep two
> > database files, one for 32 and one for 64 bit DLLs, and handle them
> > independently.  Change the magic number and you're all set.
> 
> That sounds reasonable. 'Course, you're still talking about a single
> *program* handling both, aren't you (maybe run twice, in two separate

Yes.

> 'modes'?)  Or would you actually need a *64bit* rebase.exe to handle the
> 64bit dlls, and a 32bit one to handle the 32bit dlls?

No.  It's all about how to implement it.  There's no technical reason
that a 32 bit app couldn't tweak a 64 bit DLL and vice versa.

> Maybe so, but it's not correct to apply a patch to ANY cross-platform
> tool that deliberately -- or uncaringly -- breaks one of those other
> platforms.  It is the responsibility of contributor to ensure (or at
> least make best effort to ensure, *without* actually compiling or
> testing on those other targets) that her patch First, Does No Harm --
> even to platforms she doesn't care about.
> 
> Otherwise, the patch should be rejected absent specific agreement
> otherwise by the maintainer(s) -- which in this case is NOT me, but Jason.
> 
> I know that you know all this...I'm just getting it on the record.

I think this goes over the top.  The patch adds new functionality
without breaking the existing one.  Therefore, the new code won't run
with additional tweaks on mingw, but it does *not* break mingw.

> > Skipping the 64 bit issue which needs another decision first, I don't
> > understand the question.  Whatever the sizeof img_info_t is, it's a
> > constant expression on the system for which rebase has been compiled.
> 
> You're assuming that nobody will try to use cygwin's rebase on their
> C:\MinGW tree, or their mingw rebase on their cygwin tree, or some other
> combination.
> 
> Sure, it'd be stupid, don't do that (*, **). AND most likely the "mingw
> rebase" would actually be using a *different db file* (C:/MinGW/etc/foo
> vs C:/cygwin/etc/foo) than the cygwin one, even if the targetted DLLs
> were, for instance, under the cygwin tree in both cases.

Yes, exactly.

> So...no reason to share the dbs -- so ensuring the format is "the same"
> is not important.

Exactly.

> > a) 4 bytes per record.  The entire size of the database file is just
> >    a tiny bit over 20K on my machine, with 311 DLLs installed, most of
> >    it the string table.  When was the last time you had to worry about
> >    a waste of 1244 bytes on your hard drive?
> 
> Not worried about on-disk *size* per se -- just extra *time*.  Of
> course, reading/writing it all in a big chunk direct to a memory buffer
> -- even if 4 bytes per entry is "wasted" -- is so much faster than
> iterating thru the data structure and only writing/reading the
> "necessary" fields one. at. a. time. for. each. entry. iteratively. --
> that your way is still faster.
> 
> Heck, opening all the dll's to get their actual image base and size data
> takes longer.

Yup.

> > b) Why do you want to look into the file?  That's what rebase -s -i
> >    is for.
> > 
> >> Maybe if rebase.exe had an explicit "--dump-db-for-debuggging" option
> > 
> > Still, rebase -s -i
> 
> But I thought that would, in the output, present the current DLL data in
> preference to the database contents -- so you never REALLY know what is
> stored IN the db.

Right, and there's usually no reason to.

> > Again, why would you want to do that?  There's no information to gain.
> 
> Debugging only. You've never had to inspect raw database entries as part
> of tracking down a problem?

Uhm... if so, it was looong ago.  
> 
> > rebase -s -i prints the database content, unless the real DLL differs
> > from the database storage.  If so, and if DLLs now overlap, you get
> > the information printed on the screen.  That's what you need to decide
> > if some more rebasing might be necessary.  What crucial information do
> > you gain from printing the database as is, if it doesn't match reality?
> 
> Absolutely agreed that USERS should never care about anything but -s -i.
>  I'm only thinking of rebase developers, if/when there is some
> hard-to-track problem and you need a sanity check.
> 
> What if a user's db gets scrogged -- how can you tell?  rebase -s -i

My dictionary doesn't know the word "scrogged", but I guess it's clear from
the context :)

If the db is broken, rebase overwrites it in the next run.  If the db is
broken, somebody broke it externally.  The database format is so simple,
it's quite tricky to break it from within rebase.

> Maybe the db impl should be in a separate source file, and then we could
> have a separate, very dumb tool, that just dumps the contents. It'd be
> packaged in a "rebase-devel" package and nobody (but you, me, and Jason,
> and some user who we are trying to help) would ever install it...

Well, it's not much of a problem to use the -v flag as an indicator not
to run the complicated preparation stuff in print_image_info.  If you
insist, I'll add that.  I don't think that debugging the file content is
useful but it's fine with me.

> because what you're REALLY asking is
> 	"Can I add new functionality for cygwin, that will make
> 	 rebase.exe either non- or sub-functional (or worse,
> 	 non-compilable) on msys and mingw" -- until somebody else
> 	 makes a separate cleanup patch for those platforms?"

You're kidding, right?  I don't like the way how you imply that I try
deliberately to break other targets.  That's the second time in this
email.  As you saw in my previous patches I even created msys and mingw
specific code.  The given functionality of rebase is not changed and
runs still fine on msys and mingw.  I #ifdef'ed the Cygwin stuff for a
reason and even this patch contains special #ifdefs for msys and mingw.
I think I'm far from being amused :(


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]