Menu

macro checks around #pragma pack()

BSzili
2013-10-22
2013-10-26
1 2 3 > >> (Page 1 of 3)
  • BSzili

    BSzili - 2013-10-22

    I wanted to commit my AROS ARM changes, but I noticed something I want to discuss first. There are a few checks around the #pragma packs:

    #if (defined(__PPC__) || defined(__i386__)) && defined(__GNUC__)
    

    I think these are redundant, because we only support GCC, and the structs should be 2-byte packed everywhere to assure binary compatibility. Instead of adding yet another architecture to the list (arm), can I remove the that #if altogether?

     
  • kas1e

    kas1e - 2013-10-22

    @BSzili
    I for myself know only that without pragma packs ppc versions just borks heavy in all possible places, while on 68k without pragma packs all works fine. I do not know if we should align everything for 68k too..Besides, maybe someday someone will in interest to use not only gcc again, its always better to have source where those necessary parts ifdefed.. Also with such ifdefs, pretty visibly where pragma packs starts and ends. While if we will remove them, we can someday just didn't notice some of them..

    Imho of course, but maybe adding is better..

    @Xenic
    What you think ?

     
  • BSzili

    BSzili - 2013-10-22

    @kas1e:
    You are putting words into my mouth again. Nobody talked about removing the pragma packs. I said the exact opposite: having the pragma packs by default, because it has to be 2 bytes packed anyway. Why would we need that ifdef to see where the packing starts and ends? You do a grep for #pragma pack and you can see where it begins and ends.

     
  • kas1e

    kas1e - 2013-10-22

    @BSzili

    Nobody talked about removing the pragma packs.

    And i don't talk about it either. I talk about removing of ifdefs too, not about removing of pragma packs. What i say is that why we need pragma pack for every build. For 68k build all works fine without pragma pack. Why we should pragma pack 68k version too and make it be aligned when its not need it for it ?

    You do a grep for #pragma pack and you can see where it begins and ends.

    Sure, just visually it can be unnoticed when someone new will come on board and works. Itix back in past start to adding pragma packs without ifdefs, and that cause us a lot of problems, which i fix in days, by tracebacking that he forget somewhere to do it back. And i spend a lot of time to make it all good looking with those ifdefs, so it always noticeable where it start, and where it ends, as well as it didn't pack on 68k.

     

    Last edit: kas1e 2013-10-22
  • BSzili

    BSzili - 2013-10-22

    @kas1e
    Then I misunderstood your first sentence, because you mentioned problems without #pragma pack(2).
    I meant to remove the #if to make the code simpler, and to avoid mass-commits when a new architecture is added to the supported list. #pragma pack(2) is a no-op on 68k, so why is a problem if it's not ifdef-ed out? I also don't get the reasoning behind being more noticeable. It's more verbose for sure, but what's the difference between grepping for "#pragma pack" or "#if (defined(PPC) || defined(i386)) && defined(GNUC)"?

     
  • kas1e

    kas1e - 2013-10-22

    @BSzili
    If it will make our live easy and its no-op on 68k as well, then why not, let's remove it. Just make it plz as different commits (one for remove , so if anything by any of reasons will going wrong, we will see where they all was), and another ones for arm, etc.

     
  • BSzili

    BSzili - 2013-10-22

    @kas1e
    That's what I meant, GCC on 68k already has 2 byte packing, so setting it to 2 bytes changes nothing there. I'll do a separate commit, so it can be tested just to be safe. The next thing will be the makefiles. I'll be going with your proposed solution, e.g. adding a 2nd parameter for the arch, so the make commands will be "make aros i386", "make aros arm", etc.

     
  • xenic

    xenic - 2013-10-22

    @BSzili
    The names in the makefiles need to be a single word like "aros-arm" because make will consider seperate words as additional targets.

    I'm not convinced that eliminating the conditional statements (#if defined) for #pragma pack is a good idea for the OS3 compile. The OS3 GCC compiler is 15+ years old and I've never seen anything compiled for OS3 that is "#pragma packed". Is the change necessary for AROS? If not and it's just a cosmetic change, I'd say leave it alone until we have a final bug-free OS3 release. Then we can do all the cleanup we want.

     
  • BSzili

    BSzili - 2013-10-22

    @xenic
    What if I add "arch=nnn" as a global environment variable, just like debug?
    I'll test if pragma pack works with GCC 2.95. If it's ignored that's good too, because structs already have the correct alignment.

     
  • xenic

    xenic - 2013-10-22

    @BSzili
    I need a little clarification on your "arch=nnn" suggestion. Do you mean exporting "arch=nnn" down to the lower level makefiles so that they can do something different in the makefile or actually setting an environmental variable (setenv arch nnn) to be read by the makefiles or compiler?

    Are you going to add seperate makefiles for "aros arm" or do you mean the current aros makefiles will just act differently based on the "arch=nnn" variable?

    EDIT: I suppose the conditional statements for #pragma pack might only be needed if the includes are designed to be used by SASC or VBCC or some other compiler. If GCC 2.95 ignores the #pragma pack stuff then I guess it's O.K. to change. Please do test with GCC 2.95 before committing the changed code though.

     

    Last edit: xenic 2013-10-22
  • BSzili

    BSzili - 2013-10-22

    I plan to use the existing AROS makefiles. I would put "export arch" into the top-level makefiles, and add this to the AROS makefiles:

    ifdef arch
    ARCH := $(arch)
    else
    ARCH := $(shell uname -m)
    endif
    
     
  • xenic

    xenic - 2013-10-22

    @BSzili
    O.K. We'll also need to define a variable in the makefile so that the platform designation in the version strings will indicate the correct archetecture for AROS. Just so you don't need to edit the makefiles twice, we should decide on how you want to accomplish the versioning before you start.

    There is this kind of definition in version/dopus_version.h:

    #else
        #if defined(__amigaos3__)
            #define PLATFORM [OS3]
        #elif defined(__amigaos4__)
            #define PLATFORM [OS4]
        #elif defined(__MORPHOS__)
            #define PLATFORM [MOS]
        #else
            #define PLATFORM [AROS]
        #endif
    #endif
    

    We need to put a definition in your makefile like "_D__AROSARM__" or something similar so we can add something like this to the versioning:

            #elif defined(__AROSARM__)
                #define PLATFORM [AROS-ARM]
            #else
                #define PLATFORM [AROS-x386]
            #endif
    

    I just thought it might save you a second makefile rewrite if you take the versioning into consideration.

     
  • BSzili

    BSzili - 2013-10-23

    @xenic
    OK, I'll add architecture to the platform string.

     
  • BSzili

    BSzili - 2013-10-23

    Back to the pragma pack. I managed to botch my cross compiler by trying to replace 3.4.0 with 2.95.3, so I can't link executables due to a missing library. I could compile the attached file into an object file without problems, so the directive is either recognized or ignored. Either way is good, because the alignment stays the same. You can test the file below yourself.

     
  • kas1e

    kas1e - 2013-10-23

    @BSZili
    I got pretty helpfull answer from Thore about, which i just repost with his agreement here:

    Usually the 68k ABI should align structures to 2 byte boundaries, while the PPC
    ABI uses 4 bytes by default. This is the reason why structures being used by 68k
    and PPC code must be aliged to 2 bytes to ensure compatibility. This is also the
    reason why als the AmigaOS4 SDK header files use the "#pragma pack(2)" for PPC
    code only. Furthermore this seems to be a GCC specific pragma. VBCC uses
    "#pragma amiga-align" to enforce the 2 byte alignment. But this also means that
    doing the same for 68k code does not cause any harm, as you are changing
    nothing.
    
    The reason why this aligment was chosen is that the old and ancient MC68000 could
    not do a 16bit or 32bit access to odd addresses. Hence to avoid having a ULONG
    start at an odd address the 16bit=2byte alignment was required.
    
    I'd say you can easily leave out AROS from this picture completely, because AROS
    cannot run binaries for alien architectures (i.e. AROS-i386 runnning AROS-ppc or
    AROS-x86_64 code). Only AmigaOS4 and MorphOS can do this.
    
    As I said already, a specific and fixed alignment is only required if you intend
    to mix binaries for different architectures, like 68k and PPC. If the AmigaOS4
    version will be intended to be used with PPC binaries only then all these
    pragmas will not be needed. But then old plugins will no longer work.
    

    And in case with os4 and mos, we mix 68k/ppc bins/libs/etc.

     

    Last edit: kas1e 2013-10-23
  • xenic

    xenic - 2013-10-23

    @BSzili
    It looks like my concerns about packing 68k were unwarrented. The change will be O.K. if you want to proceed. Your test file worked as expected.

    I'm not sure if you can run 68k under emulation or if you might find your missing GCC 2.95 lib, but I placed a minimal GeekGadgets GCC compiler in the repository in the "branches/68k-GG_compiler" directory. It's the one I used on my classic Amiga (A4000) and the one I run under OS4 emulation to compile 68k binaries of Dopus5.

     
  • BSzili

    BSzili - 2013-10-23

    @kas1e
    It's a neat detailed explanation, but not really related to what we were discussing here. The question was whether the GCC 2.95.3 preprocessor can interpret #pragma pack, or ignore it without throwing an error. The statement has no effect in either way, because it changes the alignment from 2 bytes to.. well, 2 bytes.

    @all
    I committed the makefiles, but the version string doesn't expands correctly, so I commented it out.

     
  • xenic

    xenic - 2013-10-23

    @BSzilli
    I checked out your changes. You don't need to do anything to the files in the Include/dopus directory. Kas1e consolidated all the necessary stuff from those files into Include/libraries/dopus5.h. If kas1e agrees we can move that directory to Misc/Unused or get rid of them altogether.

    I'm going to change the makefiles to accomodate the automatic overnight compiles. I'll let you know what changes I'm making as soon as I determine what I need to do.

     
  • xenic

    xenic - 2013-10-23

    @kas1e
    You need to temporarily remove the AROS build from your overnight build script until I can fix the makefiles to accomodate the overnight build.

    Can we remove the Include/dopus directory to avoid confusion?

     
  • BSzili

    BSzili - 2013-10-23

    @xenic
    I know that some of those headers are unused, but I couldn't remember which ones. I used a mass replace tool, so it wasn't any plus work, but they should be moved into unused to avoid unnecessary commits in the future.

     
  • kas1e

    kas1e - 2013-10-24

    @all
    Yep, aros nightlys for now didn't works:

    make: *** No rule to make target `i386-aros'.  Stop.
    

    "make aros arch=i386" seems works. Will try to build with that one.

    I also seems need to install now ARM aros cross-compiler, so to be able to build nightlies for it too.

    As for include/dopus5 , i just keep it there only because we do nothing in terms of dopus5-SDK at moment, but i just fear some of structs/defines which we do not use in the dopus itself, can be used from those include/dopus5.

    Also some files from includes/dopus5, are used. For example dopus/hooks.h used in the ftp, themes and xadopus modules. dopus/modules.h uses in the xadopus.module (but that one can be replaced).

    In general, we can just put content of dopus/hooks.h to the Include/libraries/dopus5.h , and later just all the content of those include/dopus/ files put to it, to avoid any confusions and problems.

    @Xenic

    until I can fix the makefiles

    Btw, can you also add to not put to the os3, mos and aros release archive that WBStartup/DoWbStartup and Wbtartup/DowWbStartup.info binaries , as they only for os4 (that binary just to sort the problems on os4 with new way of workign with wbstartups and co)

     

    Last edit: kas1e 2013-10-24
  • BSzili

    BSzili - 2013-10-24

    @kas1e I was about to say I the arch string is no longer part of the target, but you beat me to it :P I'm going to commit some more code to address the arosc.library changes of the ABIv0-on-trunk SDK, and then we will have armhf nighties too.

     
  • kas1e

    kas1e - 2013-10-24

    @BSzili
    Still there some error in makefile, its create directory bin.i386-aros when put all the stuff once it compiled, and then, for creating an archive we have:

    >>>>>Creating release archive: Dopus5_90_dev_aros_debug.lha
    cp: cannot stat `bin.aros/dopus5.library': No such file or directory
    make: *** [release] Error 1
    

    (i.e. tried to get it from bin.aros instead)

     
  • BSzili

    BSzili - 2013-10-24

    @kas1e
    I know about that, but xenic said he will fix that, so I'd rather not meddle with it.

     
  • xenic

    xenic - 2013-10-24

    @kas1e
    Will you relax. We knew that the upper level makefiles would need changes. I'm working on the makefiles and versioning for the aros changes. I'll finish them today.

     
1 2 3 > >> (Page 1 of 3)

Log in to post a comment.

MongoDB Logo MongoDB