Login
Username:

Password:

Remember me



Lost Password?

Register now!

Sections

Who's Online
50 user(s) are online (32 user(s) are browsing Forums)

Members: 2
Guests: 48

walkero, mr2, more...

Headlines

 
  Register To Post
(1) 2 »

let's deal with remaining bugs in dopus5 together
Home away from home
Home away from home


See User information
While Itix are busy and Xenic on vacation, i am porting all the dopus5 modules, with all your help fix __alignment issues, few bugs here and here , but still there is some left, which i do not want to hold till Ikka/Xenic will have time to look at, so maybe it will be better and faster if i will point out on them, show the code, and you all can help to fix those bugs.

Let's start from the first one. It is 2 bugs in filetype.module. One of them is our-port-os4-native (do not happens on ported os3 version). Another one are coming in module by default (i.e. even original os3-sasc version have that bug. can be as bug in the original code, and as differences between os3/os4)

There is source file of filetype module:
https://sourceforge.net/p/dopus5allami ... iletype_module/filetype.c

(there is some more, but in general all main code of module here).

So, first bug should be more or less easy one: garbled characters in some parts. Check screenshot.

Resized Image

You can see in small filetype_creator window fancy characters like something wrong with copy of strings buffer or so.

Second bug which come in original code, is happens like this:

1. run dopus5
2. dbl-click on any .guide file, sniffer will popup, choice "Sniff!"
3. in new window choice "create"
4. in new window choice "save"
5. you will have now window "filetype 'untitile' exists. do you wish to replace it ?" : press Chancel. Title screen bar will blink by black color, like there no action was.
6. press "chancel" in filetype creator window = > GR with such stack trace:


Stack trace:
(0x637B2E40) [config_close.c:189] L_FreeFiletypeList()+0x2c (section 1 @ 0x35B44)
(0x637B2E50) [aos4_ppc_libstubs.c:925] libstub_L_FreeFiletypeList()+0x14 (section 1 @ 0x9020)
(0x637B2E60) [filetype.c:680] finder_creator_proc_code()+0x2d0 (section 1 @ 0x5B7C)
(0x637B2F90) native kernel module dos.library.kmod+0x00023434
(0x637B2FC0) native kernel module kernel+0x0006ac70
(0x637B2FD0) native kernel module kernel+0x0006acf0


DAR=0x00000000, what mean it's uninitiated list header (NULL).

filetype.c:680 code is just "FreeFiletypeList( ftl );", which is Library function "L_FreeFiletypeList", and code of which from config_close.c:189 of library looks like this:

188// Free filetypes in list
189: for (node=list->filetype_list.lh_Head;node->ln_Succ;)
190: {
191next=node->ln_Succ;
192L_FreeFiletype((Cfg_Filetype *)node);
193node=next;
194: }


I do test the same code on morphos, and there is no problems. Its reacts the same (i.e. blank blinks like nothing happens), but no crash when i press chancel in main window.

Any ideas/hints/help/or-better-ready-to-compile-code very welcome :)


Go to top
Re: let's deal with remaining bugs in dopus5 together
Just popping in
Just popping in


See User information
Hi kas1e

Would love to help, but does this thing compile with a cross-compiler right now? If so then I can offer you some help with porting and compiling.

Regarding your second error below, has the list been initialised correctly with NewList()?

Let me check the source out and see if I can get a better look at it.

Daniel

Go to top
Re: let's deal with remaining bugs in dopus5 together
Home away from home
Home away from home


See User information
..


Edited by kas1e on 2013/5/30 11:10:04
Go to top
Re: let's deal with remaining bugs in dopus5 together
Just popping in
Just popping in


See User information
Quote:
So far for last few years you only say that for all the projects without any help, sadly :)


Yes, this is true, but I have very little spare time these days. I will try and help when I can but I can't commit to lots of time. That's just the way it is unfortunately.

Go to top
Re: let's deal with remaining bugs in dopus5 together
Home away from home
Home away from home


See User information
@kas1e

If the list is unitilised node == NULL there fore node->ln_Succ = DSI on 0x00000000;

Chnage code to:

188// Free filetypes in list 
189: for (node=list->filetype_list.lh_Head;node && node->ln_Succ;) 
190: { 
191next=node->ln_Succ
192L_FreeFiletype((Cfg_Filetype *)node); 
193node=next
194: }


Note: change the for loop test to

node && node->ln_Succ

if node is NULL then the seond term is never evaulauted so no crash.

This was one of the most common bugs in AWeb at one point.


[edit]
Perhaps the list is not even allocated? In which case the node = list->filetype_list will cause the failure.

so do

if(list->filetpe_list)
{
    for (
node=list->filetype_list.lh_Head;node && node->ln_Succ;) 
    { 
        
next=node->ln_Succ
        
L_FreeFiletype((Cfg_Filetype *)node); 
        
node=next
    }
}


Edited by broadblues on 2013/5/29 22:49:18
Go to top
Re: let's deal with remaining bugs in dopus5 together
Quite a regular
Quite a regular


See User information
You should not even be using the names "lh_Head", "ln_Succ" and others within the structures. The Exec provides foolproof calls to find the first/next/previous/last node in a List, and returns NULL safely if the item asked for does not exist.

What is going to happen when we change the contents of a list node? Your port is going to fail. Use the provided Exec calls like GetHead(), GetSucc(), etc, and remain future-proof.

The loop should read:

for (node = IExec->GetHead(filetype_list;
node != NULL;
node = IExec->GetSucc(node))
{
L_FreeFiletype((Cfg_Filetype *)node);
}

cheers
tony
Go to top
Re: let's deal with remaining bugs in dopus5 together
Home away from home
Home away from home


See User information
@Andy

If i do:

if(list->filetype_list
    { 
        for (
node=list->filetype_list.lh_Head;node && node->ln_Succ;)  
        {  
            
next=node->ln_Succ;  
            
L_FreeFiletype((Cfg_Filetype *)node);  
            
node=next;  
        } 
    }


Then on first line it says:

Quote:

config_close.c:197: error: used struct type value where scalar is required


If i remove it, and keep only:

for (node=list->filetype_list.lh_Head;node && node->ln_Succ;)  
    {  
        
next=node->ln_Succ;  
        
L_FreeFiletype((Cfg_Filetype *)node);  
        
node=next;  
    }


Then i have no hardcore crash , but memguard hit which point out on "FreeVec(6FF6B0A8 [, 0]) from filetype.module itself. Maybe is this FreeFiletypeList( ftl ); on line 680 of filetype.c in filetype.module should be fixed instead of library function ?

@Tonyw

Your code bring bunch of errors:

>>>>>Compiling config_close.c
config_close
.cIn function 'L_FreeFiletypeList':
config_close.c:207error'filetype_list' undeclared (first use in this function)
config_close.c:207error: (Each undeclared identifier is reported only once
config_close
.c:207error: for each function it appears in.)
config_close.c:207errorexpected ')' before ';' token
config_close
.c:208errorexpected ';' before '{' token
config_close
.c:215errorexpected expression before '}' token
config_close
.c:215errorexpected expression before '}' token
config_close
.c:215errorexpected expression before '}' token
config_close
.c:183warningunused variable 'next'
make: *** [config_close.oError 1


And in general we need to keep it for os3/mos/aros as well, so the less specific changes the better.

Quote:

What is going to happen when we change the contents of a list node? Your port is going to fail.


There is many places where dopus5 can fail if anything will changes. We need now bug-free version of what we already have, before adding and change anything in the code which can be not changed for a while and which can go to todo/improvements later


Edited by kas1e on 2013/5/30 5:20:36
Edited by kas1e on 2013/5/30 5:35:51
Edited by kas1e on 2013/5/30 11:14:47
Go to top
Re: let's deal with remaining bugs in dopus5 together
Just popping in
Just popping in


See User information
@kas1e

You know what Kas1e, sod it with trying to help you.

I've checked out, tidied up, and tested a sh*t load of code in that filetype.module source code and you're not even going to give me write-access to the repository "in case I break something".

I even refactored that 'Free Nodes' code that's already been mentioned above with proper GetSucc/GetHead etc.

Why ask for help when all you want me to do is send you source code fragments in an email or post them on to a forum. Even if I do break it, which is pretty much impossible, then the whole point of SVN is to roll back to a different version.

Good luck keeping your precious source code to yourself!

Go to top
Re: let's deal with remaining bugs in dopus5 together
Home away from home
Home away from home


See User information
..


Edited by kas1e on 2013/5/30 11:10:34
Go to top
Re: let's deal with remaining bugs in dopus5 together
Just popping in
Just popping in


See User information
Quote:
You know what Slash, sod to see how you trying to help.


I'm trying to help, by spending some of my very little, precious, spare time fixing source code for you!

Quote:
If person do nothing, why we should keep him ? Of course we keep only active ones, to not make ppls assume that another ones works on it, while they not.


If you only keep "active" ones, who clearly have more spare time than some other people (who have 12 hour day full time jobs, families etc) then why come begging on a forum for help.

Do you pay me £60k a year to work on your source code? If you want to do so, then yeah, I'll quit my job and put the effort into your "projects".

If you want a bit of help now and again, which you're always asking for, to tidy source code, fix bugs and refactor small pieces of code to make the project better then I was willing to do that as and when I could.

Yes, it might not be as often as you liked, but that's the offer was, and that's the way it's got to be.

However, as I said in my PM, that offer is now rescinded!

Quote:
And its not my code, i just to not want that code on which 3 persons works for half of year, was "refactored" by someone who even didn't discuss it with another ones, and do not think if it should be os3/mos/aros compatibility, but just want to commit.


I'm not as stupid to commit something that I'm likely to break, I know fine well that adding a couple of GetHead/GetSucc calls is NOT going to break os3/mos/aros compatibility!

I fixed nigh-on 40 warnings, on top of other things, which wouldn't make the slightest difference to the compatibility.

What about all of the source code you blindly commit to the repository, copying and pasting from snippets of code posted on a forum?

On a final note, no one is crying, I'm way passed caring about your port now, so you can please yourself what you do with the source code. For me, as I have an interest in Magellan, I think I'll just work on a local OS4 version in my spare time to see what I can come up with.

Happy coding!

Go to top
Re: let's deal with remaining bugs in dopus5 together
Home away from home
Home away from home


See User information
..

Go to top
Re: let's deal with remaining bugs in dopus5 together
Amigans Defender
Amigans Defender


See User information
back on topic please or otherwise i will close the post. Use PM if you want discuss these kind of things.

i'm really tired...
Go to top
Re: let's deal with remaining bugs in dopus5 together
Home away from home
Home away from home


See User information
..

Go to top
Re: let's deal with remaining bugs in dopus5 together
Amigans Defender
Amigans Defender


See User information
yes it is possible but i won't do. i don't like to delete other posts. You can always edit your posts.

i'm really tired...
Go to top
Re: let's deal with remaining bugs in dopus5 together
Home away from home
Home away from home


See User information
Because of your "discussion" with Slash (and the weird way this site only lets you see the last few posts in the thread when replying and not the actual post you are trying reply to!) I can't quote the relevant bits but:

The error about scaler verses struct is because I misread the code and the list->filetype_list is obviously an embedded list not a pointer to a list.

The fix I suggested makes the code more robust in case of error (hence no GR now), so you should keep it, but if you get a MemGuard hit then obviously there is an error further up the chain. It's not at all obvious from reading the code (in the time I have I can't do a build to add debug etc). I would suggest fixing the warnings if any and see what it shows. Examine them carefully before fixing, they may show the location of the bad memory allocation / freeing.





Go to top
Re: let's deal with remaining bugs in dopus5 together
Just popping in
Just popping in


See User information
Quote:

tonyw wrote:

The loop should read:

for (node = IExec->GetHead(filetype_list;
node != NULL;
node = IExec->GetSucc(node))
{
L_FreeFiletype((Cfg_Filetype *)node);
}


This code is dangerous. You will note that the iteration statement of the for() loop uses the node pointer after you have freed it. Other problems aside, the original code had some protection against this. There are other loop constructs that would be better suited, but for minimal changes:

for (node IExec->GetHead(filetype_list); node != NULL;) {
  
next IExec->GetSucc(node);
  
L_FreeFiletype((Cfg_Filetype *)node);
  
node next;
}


Go to top
Re: let's deal with remaining bugs in dopus5 together
Home away from home
Home away from home


See User information
@Karlos

Thanks as well, but about all that GetHead/GetSucc and co, there is what ikka says about, and i kind of agree on this:
https://sourceforge.net/p/dopus5allami ... sion/dev/thread/82cdef52/

Relevant part:

Quote:

Using those convenience functions make code easier to understand but it is not really helping us at all. It doesn't make our code future proof, but just creates more work because we must implement those macros to 68k builds.

Our focus is not making code look beautiful. Our focus is making it functional with GCC and PPC platforms and on AROS, too, when everything is ready. It is just working on redundant things and belong to wish list category of future enhancements and so on.


I.e. now it is no problem for make code looks "good". With all those replacements and other non-necessary stuff (right now it is not necessary for sure).

Of course even if someone will says that those macroses avail on 68k, that will be no differences, as point it just to not make work we do not need right now and to not jump on problems which we not have right now.

Our problems now its just about few bugs which need to be fixed before public release, without re-factoring of code totally, without rewriting parts which make no sense to rewrite now. All that crap can go for todo/requests.

@Andy
Yep, starting to fix warnings.


Edited by kas1e on 2013/6/1 15:35:15
Edited by kas1e on 2013/6/1 15:38:14
Go to top
Re: let's deal with remaining bugs in dopus5 together
Supreme Council
Supreme Council


See User information
GetHead, GetTail, GetSucc and GetPred are all there for a reason. On OS4, it is highly likely the internal structure of a "node" may change in the future. With this in mind, accessing the fields of the structure will break any program when those changes are made. By using the API functions, your program will continue to work even after the node changes, it might even become a VOID *, who knows.

In any case, 68k is dead, so it makes no difference there, but on OS4 it's worth it to use the API. It goes without saying that creating a few macros for 68K source compatibility is a trivial job, and one that would be worth it in the log run.

I understand you want to "get it working" now, but in the long run you are creating more work, because someone is going to have to go through the code later and "fix" these issues.

Simon

Comments made in any post are personal opinion, and are in no-way representative of any commercial entity unless specifically stated as such.
----
http://codebench.co.uk
Go to top
Re: let's deal with remaining bugs in dopus5 together
Home away from home
Home away from home


See User information
@Rigo

Well if you do you need to recompile the program, macros wont replace them self.

Unless you want drop legacy you need a second method or new types of node and list structures that has northing to do whit old, that can live side by side of legacy.

(NutsAboutAmiga)

Basilisk II for AmigaOS4
AmigaInputAnywhere
Excalibur
and other tools and apps.
Go to top
Re: let's deal with remaining bugs in dopus5 together
Home away from home
Home away from home


See User information
@Karlos
Is it dangerous? Only if the list is shared, we don't know that from the what is posted here.

If its NOT a OS structure it should be protected by Mutex.

@Kas1e
If its a sheard and if its not protected its, a danger that the a node might be deleted, during the for loop, or one of the ln_Succ pointer gets changed during the for loop.

(NutsAboutAmiga)

Basilisk II for AmigaOS4
AmigaInputAnywhere
Excalibur
and other tools and apps.
Go to top

  Register To Post
(1) 2 »

 




Currently Active Users Viewing This Thread: 1 ( 0 members and 1 Anonymous Users )




Powered by XOOPS 2.0 © 2001-2016 The XOOPS Project