forums.ps2dev.org Forum Index forums.ps2dev.org
Homebrew PS2, PSP & PS3 Development Discussions
 
 FAQFAQ   SearchSearch   MemberlistMemberlist   UsergroupsUsergroups   RegisterRegister 
 ProfileProfile   Log in to check your private messagesLog in to check your private messages   Log inLog in 

fseek regression fix & crt0.s fix & more

 
Post new topic   Reply to topic    forums.ps2dev.org Forum Index -> Patch Submissions
View previous topic :: View next topic  
Author Message
ragnarok2040



Joined: 09 Aug 2006
Posts: 230
PostPosted: Tue Jan 13, 2009 4:20 pm    Post subject: fseek regression fix & crt0.s fix & more Reply with quote
radad discovered that the crt0.s didn't use the ps2sdk definitions of ps2sdk_args_parse, ps2sdk_libc_init, and ps2sdk_libc_deinit under some circumstances when linking with ps2sdk. I replaced the .weak definitions with .weakext _ps2sdk_weak_symbol, _ps2sdk_symbol, where the weak symbol will have the same value as the _ps2sdk_symbol, otherwise if it can't find it, defines the _weak_symbol instead.

When using the libc regression tests, I realized fseek was still broken for seeks seeking SEEK_SET from offset 0, since it returned 0 and it checks for a return value > 0. Then, while testing the fix for that, I discovered that seeking backwards was broken since fioLseek can only seek forwards, so I implemented that functionality. The regression test now passes :D.

It seems I forgot to include the -fno-builtin-strncmp flag in the Makefile for erl-loader so I included that.

I also redefined pfsDopen as an alias of pfsOpen and defined hddDopen as an alias of hddOpen.

http://homebrew.thewaffleiron.net/ragnarok2040/ps2sdk.patch.tar.gz
Back to top
View user's profile Send private message
radad



Joined: 19 May 2004
Posts: 246
Location: Melbourne, Australia
PostPosted: Tue Jan 13, 2009 6:40 pm    Post subject: Reply with quote
Your fix to lseek looks like it is a bug in the file system driver not in libc. I guess that was tested using host: and ps2link?
Back to top
View user's profile Send private message
ragnarok2040



Joined: 09 Aug 2006
Posts: 230
PostPosted: Tue Jan 13, 2009 11:06 pm    Post subject: Reply with quote
Ahh, yeah. I'm still not used to the underlying functionality changing depending on the device driver used. I thought it might have been a bug caused by an underlying kernel function, or something. I didn't think to actually check ps2link's "host:" device code & ps2client's response code, :D.

I reverted my changes back to "if (ret >= 0)" in fseek() since lseek can return 0 if seeking to the beginning offset from the SEEK_CUR position or seeking offset 0 from the SEEK_SET position.

I also changed this line in ftell:
if ((n = _ps2sdk_lseek(stream->fd, 0, SEEK_CUR) >= 0))
to
if ((n = _ps2sdk_lseek(stream->fd, 0, SEEK_CUR)) >= 0)

I think I misplaced that parentheses there when I converted it to return errno which made it return 1 when it should return 0.

I've found where the problem seems to appear, though. I'll see if I can't find a solution :D.
Back to top
View user's profile Send private message
ragnarok2040



Joined: 09 Aug 2006
Posts: 230
PostPosted: Wed Jan 14, 2009 1:06 pm    Post subject: Reply with quote
Seems to be a bug in ps2client, probably some sort of difference between implementations of lseek among windows/linux. I believe using a negative offset in lseek was probably either returning a negative offset in the file descriptor, or setting EOF, then read would encounter it and return EOF. I managed to fix it using the fix I originally designed for ps2sdk, even including the unsigned int tweak that allows for larger files. I uploaded the modified ps2sdk patch before, but I forgot to mention it.

Code:

open fd = 2
lseek result = 0 //fseek 0
lseek result = 0 //ftell
tell says 0      //pre-fread
read result = 2  //fread 2
                 //offset = 2
lseek result = 4 //fseek +2
                 //offset = 4
lseek result = 4 //ftell
tell says 4      //pre-fread
read result = 2  //fread 2
                 //offset = 6
lseek result = 4 //fseek -2
                 //offset = 4
lseek result = 4 //ftell
tell says 4      //pre-fread
read result = 2  //fread 2
                 //offset = 6
lseek result = 12 //fseek SEEK_END
                  //offset = 12
lseek result = 12 //ftell


I made the fix using ps2client for uLE rev8. I'm not sure if the latest revision in svn suffers from the same problem, but I think it might.
Code:

 int ps2link_request_lseek(void *packet) {
  struct { unsigned int number; unsigned short length; int fd, offset, whence; } PACKED *request = packet;
  int result = -1;
  off64_t real_offset = (unsigned int)ntohl(request->offset);
  real_offset += lseek64(ntohl(request->fd), 0LL, SEEK_CUR); //tell

  // The request below is modified as suggested by EEUG to allow
  // large offsets up to (2**32-1) in size
  // Perform the request.
  switch (ntohl(request->whence)) {
    case SEEK_SET:
    case SEEK_END:
      result = lseek64(ntohl(request->fd), (off64_t)(unsigned int)ntohl(request->offset), ntohl(request->whence));
      break;
    case SEEK_CUR:
      result = lseek64(ntohl(request->fd), (off64_t)real_offset, SEEK_SET);
      break;
  }

  // Send the response.
  return ps2link_response_lseek(result);
 }
Back to top
View user's profile Send private message
radad



Joined: 19 May 2004
Posts: 246
Location: Melbourne, Australia
PostPosted: Wed Jan 14, 2009 1:17 pm    Post subject: Reply with quote
lseek is pretty standard amongst different OS implementations. They should all accept a negative seek.

My guess would be that its got something to do with unsigned casting. It really should be signed at all points along the chain.

Here you cast it to unsigned:
Code:
off64_t real_offset = (unsigned int)ntohl(request->offset);

So real_offset can never be negative. Did you try removing all of the unsigned casting in ps2link_request_lseek?
Back to top
View user's profile Send private message
radad



Joined: 19 May 2004
Posts: 246
Location: Melbourne, Australia
PostPosted: Wed Jan 14, 2009 1:36 pm    Post subject: Reply with quote
Actually ntohl is:
Code:
unsigned long ntohl(unsigned long netlong);
so casting from 'unsigned long' to 'unsigned int' isn't actually doing anything.

But then it implicitly casts from 'unsigned long' to 'off64_t'. Because you are casting into a larger bit field integer you will lose the negativity. You need to cast to a 'signed int' before cast to the larger 'off64_t'

So that line above should be:
Code:
off64_t real_offset = (int)ntohl(request->offset);


ie
Code:

off64_t real_offset_a = (unsigned int) -1;
off64_t real_offset_b = (int) -1;

In this exmaple: real_offset_a != real_offset_b
Back to top
View user's profile Send private message
ragnarok2040



Joined: 09 Aug 2006
Posts: 230
PostPosted: Wed Jan 14, 2009 1:58 pm    Post subject: Reply with quote
Yeah, but the same error occurred. I tried casting it to a signed long long, signed int, and while lseek would return offset-2 when it seeked backwards, read would still fail with EOF right afterwards even though tell (lseek(fd,0, SEEK_CUR) returned an offset of 4. I'm guessing something happens where lseek causes read to think it's at the EOF after using a negative offset.

Lseek might not use negative offsets instead converting -2 into a positive offset, like 65533 then seeking forwards from SEEK_CUR, repeating at SEEK_END until it runs out at SEEK_CUR-2. If there's an underlying FILE* stream, then the EOF error might be set... I keep trying to make a simpler theory but then they all get as convoluted as that one, and I start to think it's not possible :D. Even this one sounds bad.
Back to top
View user's profile Send private message
ragnarok2040



Joined: 09 Aug 2006
Posts: 230
PostPosted: Wed Jan 14, 2009 2:12 pm    Post subject: Reply with quote
Huh, just tried a simple (signed long long)ntohl(request->offset), and it worked fine but (off64_t) where off64_t is a long long int, didn't. I guess the (signed) part of the cast needs to be there in order to keep the signed bit with all the casting.
Back to top
View user's profile Send private message
ragnarok2040



Joined: 09 Aug 2006
Posts: 230
PostPosted: Wed Jan 14, 2009 2:21 pm    Post subject: Reply with quote
I can't seem to edit my old posts in the patch forum, but I wanted to say thanks for your help radad :D.
Back to top
View user's profile Send private message
radad



Joined: 19 May 2004
Posts: 246
Location: Melbourne, Australia
PostPosted: Wed Jan 14, 2009 2:37 pm    Post subject: Reply with quote
Thats ok I would like to get this correct also.

What do you get on your pc from this:
Code:
#include <stdio.h>

int main()
{
  printf("%lld\n", (long long)(long) -1);
  printf("%lld\n", (long long)(unsigned long) -1);
  printf("%lld\n", (long long)(signed long) -1);
  printf("%llu\n", (unsigned long long)(long) -1);
  printf("%llu\n", (unsigned long long)(unsigned long) -1);
  printf("%llu\n", (unsigned long long)(signed long) -1);
  printf("%lld\n", (signed long long)(long) -1);
  printf("%lld\n", (signed long long)(unsigned long) -1);
  printf("%lld\n", (signed long long)(signed long) -1);
  return 0;
}



I get:
Code:
-1
4294967295
-1
18446744073709551615
4294967295
18446744073709551615
-1
4294967295
-1
Back to top
View user's profile Send private message
ragnarok2040



Joined: 09 Aug 2006
Posts: 230
PostPosted: Wed Jan 14, 2009 2:43 pm    Post subject: Reply with quote
For my 64-bit root, building with 64-bit gcc:
Code:

-1
-1
-1
18446744073709551615
18446744073709551615
18446744073709551615
-1
-1
-1

For my 32-bit root, building with 32-bit gcc:
Code:

-1
4294967295
-1
18446744073709551615
4294967295
18446744073709551615
-1
4294967295
-1

I use ps2client on my 32-bit root.
Back to top
View user's profile Send private message
radad



Joined: 19 May 2004
Posts: 246
Location: Melbourne, Australia
PostPosted: Wed Jan 14, 2009 2:44 pm    Post subject: Reply with quote
I didnt notice this before but it looks like someone deliberately broke negative seeks:
Code:
  // The request below is modified as suggested by EEUG to allow
  // large offsets up to (2**32-1) in size
Back to top
View user's profile Send private message
ragnarok2040



Joined: 09 Aug 2006
Posts: 230
PostPosted: Wed Jan 14, 2009 3:33 pm    Post subject: Reply with quote
Yeah, I think it was to make it so positive lseeks could seek up to 4 GB sized files. The lseek seems to work though, returning the right offset result.

I tested it again casting (signed long long) and it failed again, probably because it signs everything.

I modified the lseek function to print values using %d and %lld when casting (long long)(unsigned int)-2 . With %d, -2 is printed, and with %lld, 4294967294 was printed. It seems the lseek64 function on linux uses -2 at some point, at least when returning the offset, then sets the real offset as 4294967294 in the file descriptor, then read() gets it, the offset is past the EOF and returns EOF.
Back to top
View user's profile Send private message
ragnarok2040



Joined: 09 Aug 2006
Posts: 230
PostPosted: Wed Jan 14, 2009 3:45 pm    Post subject: Reply with quote
I tested the result from lseek64 as a long long, to make sure it wasn't overflowing the result, and it definitely returns 4, where the old offset was 6 and the new offset sent was -2. The -2 offset gets casted as (long long)(unsigned int), which makes it 4294967294, so lseek64 should be returning something like 4294967296, not 4.
Back to top
View user's profile Send private message
radad



Joined: 19 May 2004
Posts: 246
Location: Melbourne, Australia
PostPosted: Wed Jan 14, 2009 3:49 pm    Post subject: Reply with quote
You cant just use %d or %lld, you have to match the right one with the right type of the variable you are trying to print.

Your results above show the casting is happening as expected. So there is no difference between (signed int) and (int) - as it should be.

off64_t is a 'long long' isnt it?

What this means is that when request->offset is -1:
Code:
off64_t real_offset = (unsigned int)ntohl(request->offset);

real_offset = 4294967295

but for this:
Code:
off64_t real_offset = (int)ntohl(request->offset);

real_offset = -1

lseek64 is working correctly you just have to do the casting correctly.

But because of that change for EEUG negative seeks are not supported. You can't support -1 and (2**32-1) using a a 32 bit integer.
Back to top
View user's profile Send private message
radad



Joined: 19 May 2004
Posts: 246
Location: Melbourne, Australia
PostPosted: Wed Jan 14, 2009 4:25 pm    Post subject: Reply with quote
So what do you get from this program:
Code:
#define _LARGEFILE64_SOURCE

#include <stdio.h>
#include <fcntl.h>
#include <sys/types.h>
#include <unistd.h>

//typedef long long off64_t;

int main()
{
  off64_t o = (long long)(unsigned long) -1;
  printf("%lld\n", o);
  int fd = open("test.c", 0);
  printf("fd %d\n", fd);
  printf("%lld\n", lseek64(fd, 2, SEEK_SET));
  printf("%lld\n", lseek64(fd, o, SEEK_CUR));
  printf("%lld\n", lseek64(fd, 2, SEEK_SET));
  printf("%lld\n", lseek64(fd, -1, SEEK_CUR));
  close(fd);
  return 0;
}



I get:
Code:
4294967295
fd 3
2
4294967297
2
1
Back to top
View user's profile Send private message
ragnarok2040



Joined: 09 Aug 2006
Posts: 230
PostPosted: Wed Jan 14, 2009 8:14 pm    Post subject: Reply with quote
Sorry for replying so late but I was testing my off64_t type and looking for the lseek64 prototype. Then I came across _LARGEFILE64_SOURCE in the man page for lseek64 but you'd already posted :D.

I get:
Code:
4294967295
fd 3
2
4294967297
2
1

I guess without _LARGEFILE64_SOURCE defined lseek64 is probably redefined as an alias for something else, since it returns what seems like an overflow of a long or int.

By getting rid of the define and changing it to mimic the test:
Code:
  off64_t o = (long long)(unsigned int) -2;
  printf("%llu\n", o);
  int fd = open("types.c", 0);
  printf("fd %d\n", fd);
  printf("%lld\n", lseek64(fd, 0LL, SEEK_SET));
  printf("%lld\n", lseek64(fd, 6LL, SEEK_SET));
  printf("%lld\n", lseek64(fd, o, SEEK_CUR));
  printf("%lld\n", lseek64(fd, 2LL, SEEK_SET));
  printf("%lld\n", lseek64(fd, -1LL, SEEK_CUR));
  close(fd);
  return 0;

I get:
Code:
4294967294
fd 3
0
6
4
2
-4294967295
This is the somewhat similar behavior as ps2client when using lseek64. The last couple of seeks don't seem to work properly.

I already changed the code in ps2client back to using the standard lseek, with the ntohl(request->offset) casted to int.
At least I learned something new about lseek64 on linux *_*.
Back to top
View user's profile Send private message
ooPo
Site Admin


Joined: 17 Jan 2004
Posts: 2032
Location: Canada
PostPosted: Thu Jan 15, 2009 12:54 am    Post subject: Reply with quote
ragnarok2040 wrote:
I can't seem to edit my old posts in the patch forum, but I wanted to say thanks for your help radad :D.

This should be fixed.
Back to top
View user's profile Send private message Visit poster's website
ragnarok2040



Joined: 09 Aug 2006
Posts: 230
PostPosted: Thu Jan 15, 2009 3:11 pm    Post subject: Reply with quote
Thanks ooPo, :D, there's an edit button again.

I separated the patch into four patches.
http://homebrew.thewaffleiron.net/ragnarok2040/ps2sdk-patches.tar.gz

crt0.patch:
I replaced all the .weak symbols with .weakext, including _init and _fini. They'll be replaced by the stronger symbols within ps2sdk, or use the locally defined definitions if the ps2sdk versions don't exist. This seems to fix the problem where weak crt0.s definitions were overriding ps2sdk definitions.

stdio.patch:
I added the declaration of rename() to stdio.h.
I fixed fseeks to the beginning of a file where the returned offset by fioLseek and fileXioLseek is 0.
I fixed ftells returning the wrong offset by moving the right side of a parentheses back to the proper place.

erl-loader.patch:
Included -fno-builtin-strncmp in EE_LDFLAGS in the Makefile to fix a build warning. Not sure why the warning didn't appear before.

apa.patch:
Added a hddDopen wrapper for hddOpen for the apa iop driver and included the declaration in the header. This is an optional patch.
Back to top
View user's profile Send private message
radad



Joined: 19 May 2004
Posts: 246
Location: Melbourne, Australia
PostPosted: Thu Jan 15, 2009 7:12 pm    Post subject: Reply with quote
commited to svn rev 1508 - 1511.

ragnarok2040: youve gotta get your own write access
Back to top
View user's profile Send private message
ragnarok2040



Joined: 09 Aug 2006
Posts: 230
PostPosted: Thu Jan 15, 2009 8:38 pm    Post subject: Reply with quote
I don't know about that now. I tried to submit a patch to fix negative seeks for fseek because of a problem with my ps2client :D. Thanks for helping me, again, with that. It did teach me to start utilizing man pages more often, heh. I haven't done much code involving lseek and the like, just some changes around fceultra/snes9x code to handle file handling errors more gently.

I'd appreciate access, though. The libjpeg/libpng ports are messed up from the previous patches to update them. I think patch just removed the contents of the removed files instead of removing the files and libjpeg's makefile wasn't updated, among a few other things.

My email's ragnarok2040 at gmail.com, or I can go to #ps2dev, or however it's done :D.
Back to top
View user's profile Send private message
radad



Joined: 19 May 2004
Posts: 246
Location: Melbourne, Australia
PostPosted: Thu Jan 15, 2009 9:22 pm    Post subject: Reply with quote
try sending a pm to oobles
Back to top
View user's profile Send private message
Display posts from previous:   
Post new topic   Reply to topic    forums.ps2dev.org Forum Index -> Patch Submissions All times are GMT + 10 Hours
Page 1 of 1

 
Jump to:  
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum


Powered by phpBB © 2001, 2005 phpBB Group