misc CVS patches

While trying to hunt down a codegen bug (not yet found) ...

Have you considered using bugpoint for your codegen debugging needs?
http://llvm.cs.uiuc.edu/docs/Bugpoint.html#codegendebug

I've collected some small patches you might find useful.

Sweet!

Please review and apply as you see fit.

I've applied your gccld patch.

For the isExecutable patch, you check to see if it's a file first before
calling access(), but that precludes that function being used on a
directory (which is a valid sys::Path object and could be queried for
being executable), so I think it's unnecessary. I also applied your
patch to ignore dangling symlinks.

Finally, I applied your patch to ArchiveWriter with minor modifications
-- please see llvm-commits or the web archive.

P.S. are there any plans to remove trailing whitespace from the source
files? - these cause noisy diffs and sometimes require much manual
work.

Sure. Patches are accepted, but please separate formatting changes from
functionality changes into different patches.

Thanks again!

MIsha,

You asked for my feedback on Markus’ recent patches. Here it is.

> While trying to hunt down a codegen bug (not yet found) ...

Have you considered using bugpoint for your codegen debugging needs?
[http://llvm.cs.uiuc.edu/docs/Bugpoint.html#codegendebug](http://llvm.cs.uiuc.edu/docs/Bugpoint.html#codegendebug)

> I've collected some small patches you might find useful.

Sweet!
 
> Please review and apply as you see fit.

I've applied your gccld patch.  

I concur.


For the isExecutable patch, you check to see if it's a file first before
calling access(), but that precludes that function being used on a
directory (which is a valid sys::Path object and could be queried for
being executable), so I think it's unnecessary.  

The original intent of this method was to check for executable programs, not whether a directory is searchable. If we want that capability we should add a method named “searchable”. Some operating systems don’t have that concept and we should make the interface functions on Path address only one concept at a time. So, given that the “executable” method is verifying that the path points to a program that could be executed by the calling process, Markus patch is reasonable (needed, even).

I also applied your
patch to ignore dangling symlinks.

That part of the Path.h patch is similar to a previous one we committed and I concur.


Finally, I applied your patch to ArchiveWriter with minor modifications
-- please see llvm-commits or the web archive.

I concur.

Reid.

Speaking of random formatting changes, I've noticed in a few pages that the doxygen documentation comments incorrectly only have two '//' characters on the line. Hence, no doxygen documentation gets generated. Should I be creating a patch for this, or should I just report it?

Evan

Please send patches. :slight_smile:

-Chris

BTW, for patches like this that don't generally need discussion (just someone to apply them), it's better to email llvmbugs instead of llvm-dev, just to reduce noise on llvmdev.

-Chris

Misha Brukman wrote:
>On Tue, Apr 19, 2005 at 07:01:40AM +0200, Markus F.X.J. Oberhumer
>wrote: Have you considered using bugpoint for your codegen debugging
>needs? http://llvm.cs.uiuc.edu/docs/Bugpoint.html#codegendebug

Well, the (critical) bug in question was
http://llvm.cs.uiuc.edu/bugs/show_bug.cgi?id=548 , and I'm not sure if
bugpoint could have helped me reducing the testcase down from a huge
program.

Actually, that is the `point' of bugpoint, pun intended. :slight_smile:

But then, I've never looked too closely - is there a small tutorial ?

http://llvm.cs.uiuc.edu/docs/HowToSubmitABug.html

About the only thing we haven't gotten to `bugpoint' is front-end
miscompilations -- those test cases need to be narrowed down by hand,
but once your code is in LLVM bytecode, bugpoint can rip it into shreds.

>For the isExecutable patch, you check to see if it's a file first before
>calling access(), but that precludes that function being used on a
>directory (which is a valid sys::Path object and could be queried for
>being executable), so I think it's unnecessary.

I'm pretty sure that the semantics of isExecutable() means "can I
excecute a _file_". Without this patch FindExecutable() returns
directories on $PATH - see
http://llvm.cs.uiuc.edu/bugs/show_bug.cgi?id=545 .

You and Reid have a point. I'm convinced. Patched, bug closed.

> I also applied your patch to ignore dangling symlinks.

Fine, so http://llvm.cs.uiuc.edu/bugs/show_bug.cgi?id=543 can be closed.

As per Reid's comment, I'll leave it open until this issue is resolved:

3. Figure out why ThrowErrno isn't appending the standard operating
system error message that would tell us why the status couldn't be
obtained.

>Finally, I applied your patch to ArchiveWriter with minor
>modifications -- please see llvm-commits or the web archive.

I didn't use get{u,g}id() because that's not portable - I think we
need another abstraction in the System library for these.

Hmm, seems like it's available on the Unices that we support: Linux, OS
X, and SunOS. Man page lists getgid() as conforming to POSIX and BSD
4.3, so I'm assuming *BSD has them too (can't confirm for lack of a *BSD
system). As soon as we run into a Unix system that we want to support,
we might have to go the abstraction route, but it seems to work for our
currently-supported platforms.

Do you really want external patches for this ? A simple Perl script
that runs on all *.h and *.cpp files, and a local commit from your
side would be much simpler. The testsuite should also be enhanced to
daily report files with trailing whitespace. Please note that this not
purely academic - trailing whitespace are a horror for anyone
maintaining external patches against a CVS tree.

I'll look into this.

Thanks,

In case you really prefer outside patches for this issue, here is a
first version on a small subset of the sources.

Committed.

But I'd think you would want to handle this "in house".

I think you're right, it'll be easier that way. :wink:

Misha Brukman wrote:

I didn't use get{u,g}id() because that's not portable - I think we
need another abstraction in the System library for these.
   
Hmm, seems like it's available on the Unices that we support: Linux, OS
X, and SunOS. Man page lists getgid() as conforming to POSIX and BSD
4.3, so I'm assuming *BSD has them too (can't confirm for lack of a *BSD
system). As soon as we run into a Unix system that we want to support,
we might have to go the abstraction route, but it seems to work for our
currently-supported platforms.

FreeBSD has them. According to its man pages, getuid was introduced in Version 7 AT&T Unix. Ancient history. It doesn't say when getgid was introduced, but it's just as old as it's documented in my 1980 edition Berkeley VAX UNIX manual, and in any case both are in POSIX.1 and therefore ought to be portable by definition.

I’ve committed a patch to Path.inc that (hopefully) resolves this situation. I believe the assignment to errno was not working because its a macro on some platforms. In any event, this is hard to test because you have to create error situations in the file system. I’ll get there someday but its not a large enough issue to warrant significant time investment. I also cleaned up the error messages so they all have a similar format.

Bug 543 is resolved as a consequence.

Reid.

Markus,

This patch can’t be applied. Not all platforms have getgid() and getuid() functions. Placing non-portable code outside of lib/System is deprecated. We set the values to 1000 by default because in general the uid/gid doesn’t matter in an archive and the 1000 value gets you to a safe (non-root, non-system) value. If a specific value for these is needed on a given platform, then we need to implement something like “getDefaultUserId” and “getDefaultGroupId” functions in lib/System and use those in lib/Bytecode/Archive.

Is there a specific problem that is driving these changes?

Reid.

Nope. Win32 is not (and does not pretend to be) POSIX compliant. These functions cannot be used outside of lib/System/Unix. New methods need to be added to class llvm::sys::Process.

Markus F.X.J. Oberhumer wrote:

Implemented as sys::Process::GetCurrentUserId() and sys::Process::GetCurrentGroupId(). Both Unix and Win32 implementations have been provided. Jeff, is there something more intelligent on Win32 that these methods could return (other than 65536)?.

I’ve also corrected the ArchiveWriter to use these methods when it is defaulting the archive header.

Reid

Misha pointed out this document, which has the basics:
http://llvm.cs.uiuc.edu/docs/HowToSubmitABug.html

This document contains more detail:
http://llvm.cs.uiuc.edu/docs/Bugpoint.html

Like Misha said, bugpoint can track down problems in the JIT, in the native code generators, or in optimization passes. Generally reducing the "problem" down to an individual function, a loop, a basic block, or even a few llvm instructions depending on what is necessary to reproduce the problem.

For people that are doing llvm optimizer and codegen development, I highly recommend looking into it. :slight_smile:

-Chris

Nope. Any number is as good as any other.

Reid Spencer wrote:

Whoops. Mea culpa. After the lib/System/Unix patches, I was mentally
still in Unix-land (hence all my Unix references).

Thanks for fixing, Reid! :slight_smile:

Yeah, that’s fine. I’ll change it soon.

Reid.

Dear LLVMers,

If you live on the bleeding edge (i.e. CVS version), please read!

Do you really want external patches for this ? A simple Perl script
that runs on all *.h and *.cpp files, and a local commit from your
side would be much simpler.

I'm in the process of doing just this as we speak. What this means:
please hold off updating in the mean time, unless you want to rebuild
your codebase several times, as I am about to touch a *lot* of files.

I will send a message to the list when I'm done, so you can get all the
changes (and hopefully, few conflicts)

The testsuite should also be enhanced to daily report files with
trailing whitespace.

Good idea, I'll add that to my todo list.

Please note that this not purely academic - trailing whitespace are a
horror for anyone maintaining external patches against a CVS tree.

I was not aware that this is such a big problem, so thanks for point
this out. It'll be fixed in a short while.

OK, I'll do this in the next pass over the tree.

Why not put all this into a pre-commit filter in CVS and be done with it? We’d never be bothered with it again as it would never be committed again.

Reid.

Why not put all this into a pre-commit filter in CVS and be done with
it? We'd never be bothered with it again as it would never be committed
again.

I'd rather not have CVS commit scripts mucking with the code. If you want to have the nightly tester whine about source code with spaces at the end of lines (like it whines about compiler warnings), that would be fine.

BTW, does anyone know how to tell xemacs to autodelete end of line spaces?

-Chris

Dear LLVMers,

If you live on the bleeding edge (i.e. CVS version), please read!

Do you really want external patches for this ? A simple Perl script
that runs on all *.h and *.cpp files, and a local commit from your
side would be much simpler.

I'm in the process of doing just this as we speak. What this means:
please hold off updating in the mean time, unless you want to rebuild
your codebase several times, as I am about to touch a *lot* of files.

I will send a message to the list when I'm done, so you can get all the
changes (and hopefully, few conflicts)

The testsuite should also be enhanced to daily report files with
trailing whitespace.

Good idea, I'll add that to my todo list.

Please note that this not purely academic - trailing whitespace are a
horror for anyone maintaining external patches against a CVS tree.

I was not aware that this is such a big problem, so thanks for point
this out. It'll be fixed in a short while.

_______________________
Reid Spencer
President & CTO
eXtensible Systems, Inc.
rspencer@x10sys.com

-Chris