fseek regression fix & crt0.s fix & more

[ragnarok2040]

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/ragna ... tch.tar.gz

[radad]

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?

[ragnarok2040]

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.

[ragnarok2040]

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: Select all

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: Select all

 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);
 }

[radad]

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: Select all

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?

[radad]

Actually ntohl is:

Code: Select all

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: Select all

off64_t real_offset = (int)ntohl(request->offset);

ie

Code: Select all

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

In this exmaple: real_offset_a != real_offset_b

[ragnarok2040]

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.

[ragnarok2040]

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.

[ragnarok2040]

I can't seem to edit my old posts in the patch forum, but I wanted to say thanks for your help radad :D.

[radad]

Thats ok I would like to get this correct also.

What do you get on your pc from this:

Code: Select all

#include <stdio.h>

int main&#40;&#41;
&#123;
  printf&#40;"%lld\n", &#40;long long&#41;&#40;long&#41; -1&#41;;
  printf&#40;"%lld\n", &#40;long long&#41;&#40;unsigned long&#41; -1&#41;;
  printf&#40;"%lld\n", &#40;long long&#41;&#40;signed long&#41; -1&#41;;
  printf&#40;"%llu\n", &#40;unsigned long long&#41;&#40;long&#41; -1&#41;;
  printf&#40;"%llu\n", &#40;unsigned long long&#41;&#40;unsigned long&#41; -1&#41;;
  printf&#40;"%llu\n", &#40;unsigned long long&#41;&#40;signed long&#41; -1&#41;;
  printf&#40;"%lld\n", &#40;signed long long&#41;&#40;long&#41; -1&#41;;
  printf&#40;"%lld\n", &#40;signed long long&#41;&#40;unsigned long&#41; -1&#41;;
  printf&#40;"%lld\n", &#40;signed long long&#41;&#40;signed long&#41; -1&#41;;
  return 0;
&#125;

I get:

Code: Select all

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

[ragnarok2040]

For my 64-bit root, building with 64-bit gcc:

Code: Select all

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

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

Code: Select all

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

I use ps2client on my 32-bit root.

[radad]

I didnt notice this before but it looks like someone deliberately broke negative seeks:

Code: Select all

  // The request below is modified as suggested by EEUG to allow
  // large offsets up to (2**32-1) in size

[ragnarok2040]

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.

[ragnarok2040]

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.

[radad]

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: Select all

off64_t real_offset = (unsigned int)ntohl(request->offset);

real_offset = 4294967295

but for this:

Code: Select all

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.

[radad]

So what do you get from this program:

Code: Select all

#define _LARGEFILE64_SOURCE

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

//typedef long long off64_t;

int main&#40;&#41;
&#123;
  off64_t o = &#40;long long&#41;&#40;unsigned long&#41; -1;
  printf&#40;"%lld\n", o&#41;;
  int fd = open&#40;"test.c", 0&#41;;
  printf&#40;"fd %d\n", fd&#41;;
  printf&#40;"%lld\n", lseek64&#40;fd, 2, SEEK_SET&#41;&#41;;
  printf&#40;"%lld\n", lseek64&#40;fd, o, SEEK_CUR&#41;&#41;;
  printf&#40;"%lld\n", lseek64&#40;fd, 2, SEEK_SET&#41;&#41;;
  printf&#40;"%lld\n", lseek64&#40;fd, -1, SEEK_CUR&#41;&#41;;
  close&#40;fd&#41;;
  return 0;
&#125;

I get:

Code: Select all

4294967295
fd 3
2
4294967297
2
1

[ragnarok2040]

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: Select all

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: Select all

  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: Select all

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 *_*.

[ooPo]

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.

[ragnarok2040]

Thanks ooPo, :D, there's an edit button again.

I separated the patch into four patches.
http://homebrew.thewaffleiron.net/ragna ... hes.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.

[radad]

commited to svn rev 1508 - 1511.

ragnarok2040: youve gotta get your own write access

[ragnarok2040]

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.

[radad]

try sending a pm to oobles