I build 68k version of code with that define, and when i access to the function which use that construction by running binary on os4, i have a window:
A Process called a DOS function with a non longword-aligned Anchor.
Function: "MatchFirst()"
So i need something (better if it will be just a simple define like my one), which will works everywhere and in all the cases.
No only that windows problem, but sometime that "bad" alignment cause crashes when i compile such a code as ppc one. Sometime its all ok, sometime not. But its for sure sucky to be in hope, just need to do everything right.
Currently for those dos structures itix make a macro like this:
/* Long word alignement (mainly used to get
* FIB or DISK_INFO as auto variables)
*/
#define D_S(type,name) char a_##name[sizeof(type)+3]; \
type *name = (type *)((LONG)(a_##name+3) & ~3);
So i just in code do:
// Build theme list
Att_List *theme_build_list(char *path)
{
Att_List *list;
// Create list
if (list=Att_NewList(LISTF_POOL))
{
long err;
char buf[280];
D_S(struct AnchorPath,anchor) // was with __alignment: struct AnchorPath __aligned anchor;
anchor->ap_BreakBits=0; // was with __alignment: anchor.ap_BreakBits=0;
anchor->ap_Flags=APF_DOWILD; // was with __alignment: anchor.ap_Flags=APF_DOWILD;
anchor->ap_Strlen=0; //was with __alignment: anchor.ap_Strlen=0;
strcpy(buf,path);
AddPart(buf,"#?.theme",280);
err=MatchFirst(buf,anchor); // was with __alignment: err=MatchFirst(buf,&anchor);
while (!err)
{
char *ptr;
if (anchor->ap_Info.fib_DirEntryType>0) // was with __alignment: if (anchor.ap_Info.fib_DirEntryType>0)
{
anchor->ap_Flags&=~APF_DODIR; //was with __alignment: anchor.ap_Flags&=~APF_DODIR;
continue;
}
anchor->ap_Info.fib_FileName[strlen(anchor->ap_Info.fib_FileName)-6]=0; // was with __alignment: anchor.ap_Info.fib_FileName[strlen(anchor.ap_Info.fib_FileName)-6]=0;
for (ptr=anchor->ap_Info.fib_FileName;*ptr;ptr++) // was with alignment: for (ptr=anchor.ap_Info.fib_FileName;*ptr;ptr++)
if (*ptr=='_') *ptr=' ';
Att_NewNode(list,anchor->ap_Info.fib_FileName,0,ADDNODE_SORT); // was with alignment: Att_NewNode(list,anchor.ap_Info.fib_FileName,0,ADDNODE_SORT);
err=MatchNext(anchor); // was with __ailgnment: err=MatchNext(&anchor);
}
MatchEnd(anchor); // was with __alignment: MatchEnd(&anchor);
}
return list;
}
That deals with alignment in that case, but if for example i have:
char __aligned oldname[34];
and then it used like:
// Get old filename
strcpy(oldname+1,FilePart(name));
oldname[0]=strlen(oldname+1);
Using __aligned with a char or UBYTE is nonsense IMHO. Bytes are always aligned to byte boundary. Only multi-byte datatypes need special alignment.
If you look at the code it is used for a BSTR so it needs to be 32-bit aligned. BCPL pointers always have to be 32-bit aligned no matter what they are pointing to.
That being said a better solution would probably be to just use AllocMem() or AllocVec() to allocate the buffer.
I don't see why the __aligned keyword is used here. The only alignment needed for the imagedata should be 16-bit alignment and the USHORT type should ensure this.
What OTOH could be needed is something like __chip (supported in vbcc, don't know about gcc) to ensure that it's placed in chip memory on Classic machines. If there's no such attribute for gcc you may have to use AllocMem() with MEMF_CHIP.
Maybe on SASC USHORT with/without __aligned can be different ? But if on gcc is not, then i will just remove it from there.
If it odd-aligns USHORT then it will create broken code on 68000 CPUs (the 68000 will crash if you attempt to access a word or long word at an odd memory address) and slower code on 68020+ CPUs.
Try applying the macro to the entire struct not just the emebedded fib.
A proper solution would be to change the structure to a pointer to the FileInfoBlock and allocate it with AllocDosObject(), that will require a lot of rewriting though, depending on how much this is used.
Perhaps we start to see why previous attempts to port DOpus got bogged down, if this kind of hackery is used a lot in the source.
If it odd-aligns USHORT then it will create broken code on 68000 CPUs (the 68000 will crash if you attempt to access a word or long word at an odd memory address) and slower code on 68020+ CPUs.
So on what should i change that to have all be all right:
Perhaps we start to see why previous attempts to port DOpus got bogged down, if this kind of hackery is used a lot in the source.
There is not much places where __aligned is used. Mostly that what i put there is from "modules" (some of them done without). In the library and main binary, __aligned used only for FIB/Anchor and InfoData dos variables, where is Itix's D_S macro help well. Through, there is one moment in the Library which i for now don't touch and which need fix. In the library/iff.c:
if ((parent=ParentDir(handle->iff_SafeFile)))
{
long key;
char __aligned oldname[34];
short a;
// Get old filename
strcpy(oldname+1,FilePart(name));
oldname[0]=strlen(oldname+1);
....
It is only one place in library itself where __aligned used like this, and with which i do not know how to deal properly without rewiring code. I just want to make it as much as possible with less changes for first release to avoid our own bugs. I.e. priority#1 : no changes as possible, no rewriting as possible.
In the main program binary, __aligned is used only for dos variables, so there is no strange places which can't be covered by macro. There is those changes if you in interest to see: https://sourceforge.net/p/dopus5allamigas/code/266/
Only modules in some places use it somehow strange. I.e. not much of hackery, just some suckery. And imho previous attempt to port dopus5 fail just because one person can't deal with it without much motivation and without asking help. I assume they do something, but then stuck on some part, and instead of asking help publicly, just start to jerky on yourselves skills and its all die because of that.
@All Btw, i assume that:
Quote:
struct FileInfoBlock __aligned fib={0};
And that:
Quote:
struct FileInfoBlock __aligned fib;
Are the same 1:1, except that in case with {0} its clear with NULL, and in another is not ? I mean __aligned there didn't start to be different because of that ?
Edited by kas1e on 2013/5/27 6:23:20 Edited by kas1e on 2013/5/27 6:23:59
Just remove the __aligned keyword. Also you should rather use UWORD than USHORT as they are the same except that USHORT is an obsolete typedef kept only for compatibility with old code.
@salas00 Thanks ! icon.module now fixed too. There is commit about (i just note in commit that its you fix it to raise your motivation to help us more to make it works right:) ): https://sourceforge.net/p/dopus5allamigas/code/428/
Now, what worry me pretty much is filetype.module, where __alignment uses not only for autovariables, but also for char bufs, like:
TBH I don't see why those char buffers need to be aligned except for the id_to_str() function which you may want to change if you want to remain 68000 compatible to:
char *id_to_str( LONG id, char *buf )
{
buf[0] = (id >> 24) & 0xff;
buf[1] = (id >> 16) & 0xff;
buf[2] = (id >> 8) & 0xff;
buf[3] = id & 0xff;
buf[4] = 0;
/* Long word alignement (mainly used to get
* FIB or DISK_INFO as auto variables)
*/
#define D_S(type,name) char a_##name[sizeof(type)+3]; \
type *name = (type *)((LONG)(a_##name+3) & ~3);
D_S(char, oldname) // char __aligned oldname[34];
And no more changes. I assume by this it all should works fine as it's just char now, and needs no more code changes ?
As for more characters, as its in iff.c i assume its just for name of toolbar images mostly, so we can deal with it later and extend it if there will be any needs.
So it is struct, and in struct i can't use D_S macro. But all what can i say, is that diskinfo.module works pretty well when i just align it to "4", via : #define __aligned __attribute__((aligned(4))) . Ikka suggest just replace that string on that:
Ikka just fixed the alignment, not the buffer overflow bug I pointed out. You need that larger buffer and to use strlcpy() (as Colin pointed out my usage of strncpy()was wrong), else it will quickly crash on long file names.
I was about to podt that fib = {0} will not compile but I see you got there allready.
Ikka just fixed the alignment, not the buffer overflow bug I pointed out. You need that larger buffer and to use strlcpy() (as Colin pointed out my usage of strncpy()was wrong), else it will quickly crash on long file names.
Yep, he wrote on our forum about:
Quote:
Quote:
Don't use strncpy(), it doesn't guarantee nul-termination, you want to use strlcpy() instead.
In this case it will be always NULL terminated. strlcpy() and stccpy() are always safer and faster, though. The buffer could be extended to hold longer filenames up to 108 characters.
Actually you need to suppoprt filenames up to 255 bytes.
Current dopus5 limitation is 108 characters for filenames, so if i will change it only there it will be just strange. Its something for todo like "make files names more than 108 characters works", because it will needs changes in more than one place.
News feed
OS4 Feedback forum
OS4Depot Feedback forum
Amigabounty forum
OpenAmiga forum
AmiCygnix forum
ABC shell forum
AmiKit forum
Cinnamon Writer forum
CodeBench forum
Digital Universe forum
Dopus 5 forum
E-UAE forum
Gnash forum
Ibrowse forum
JAmiga forum
Odyssey forum
OWB forum
Qt forum
SmartFileSystem forum
Timberwolf forum
TouchDevice forum
TuneNet forum
Unsatisfactory Software forum
Donate
