New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

long in struct causes incorrect align on 32-bit linux (BridJ) #320

Closed
hughperkins opened this Issue Jun 19, 2012 · 8 comments

Comments

Projects
None yet
3 participants
@hughperkins

hughperkins commented Jun 19, 2012

long in struct causes incorrect align on 32-bit linux

if I pass this struct in to a method:

typedef struct learn_parm {
  double svm_c;              
  double double3;
  long   type;               
  double double2;
} LEARN_PARM;

which method looks like:

void printLearnParm(LEARN_PARM *learn_parm ) {
    printf("svm c %lf kerneltype %ld double2 %lf double3 %lf\n", learn_parm->svm_c,
         learn_parm->type,     learn_parm->double2, learn_parm->double3 );
}

... then the first 2 doubles print correctly, whereas the third one is garbage.

I think that this is because the 'long' field causes incorrect alignment/padding for subsequent fields.

Environment: ubuntu linux 12.04, 32-bit

Note that this is still unsolved in bridj-0.6.2-20120618.225518-13.jar snapshot

Note that adding one additional long before the last double causes the last double to display correctly.

What I think is happening:

  • the project is developed on Windows
  • on Windows, alignment is to 8-bytes for doubles
  • on linux, doubles are aligned on 4-byte boundaries

( http://en.wikipedia.org/wiki/Data_structure_alignment: "A double (eight bytes) will be 8-byte aligned on Windows and 4-byte aligned on Linux (8-byte with -malign-double compile time option)." )

@hughperkins

This comment has been minimized.

Show comment
Hide comment
@hughperkins

hughperkins Jun 19, 2012

I'm guessing the problem is somewhere around lines 603-608 of StructIO.java:

        aggregatedField.alignment = Math.max(
            aggregatedField.alignment, 
            field.desc.alignment >= 0 ?
            field.desc.alignment :
            field.desc.byteLength
        );

Possibly this needs to check Platform.isLinux(), and whether the field is a double or not, and whether the platform is 32-bits, and align to 4-bytes if: isLInux() && isDouble() && is32Bits() ?

hughperkins commented Jun 19, 2012

I'm guessing the problem is somewhere around lines 603-608 of StructIO.java:

        aggregatedField.alignment = Math.max(
            aggregatedField.alignment, 
            field.desc.alignment >= 0 ?
            field.desc.alignment :
            field.desc.byteLength
        );

Possibly this needs to check Platform.isLinux(), and whether the field is a double or not, and whether the platform is 32-bits, and align to 4-bytes if: isLInux() && isDouble() && is32Bits() ?

@hughperkins

This comment has been minimized.

Show comment
Hide comment
@hughperkins

hughperkins Jun 19, 2012

Note that this issue does not exist in 64-bit (understandable since linux double aligns on 8-byte in 64-bit). It is specific to 32-bit linux.

hughperkins commented Jun 19, 2012

Note that this issue does not exist in 64-bit (understandable since linux double aligns on 8-byte in 64-bit). It is specific to 32-bit linux.

@estw

This comment has been minimized.

Show comment
Hide comment
@estw

estw Jan 2, 2013

Hi Olivier,

Did you have any thoughts on this? I have the same problem and wrote a Customizer to correct it:

public class ScoreAlignmentCustomizer extends DefaultCustomizer {
    @Override
    public void beforeLayout(StructIO io, List<AggregatedFieldDesc> aggregatedFields) {
        for (AggregatedFieldDesc field : aggregatedFields) {
            if (field.fields.get(0).toString().contains("score")) {
                if (Platform.isLinux() && !Platform.is64Bits()) {
                    field.alignment = 4;
                } else {
                    field.alignment = 8;
                }
            }
        }
    }
}

However it results in an "Unaligned pointer access" exception from BridJ. I enabled SUPPORTS_UNALIGNED_ACCESS for all platforms in bridj.hpp and it now all works correctly but obviously I'm concerned about the impact of enabling SUPPORTS_UNALIGNED_ACCESS under Linux and also the inelegance of the customizer.

I would be happy to try and update the code in StructIO and generate a pull request to avoid the Customizer but that will still result in the Unaligned error.

Any help would be greatly appreciated.

estw commented Jan 2, 2013

Hi Olivier,

Did you have any thoughts on this? I have the same problem and wrote a Customizer to correct it:

public class ScoreAlignmentCustomizer extends DefaultCustomizer {
    @Override
    public void beforeLayout(StructIO io, List<AggregatedFieldDesc> aggregatedFields) {
        for (AggregatedFieldDesc field : aggregatedFields) {
            if (field.fields.get(0).toString().contains("score")) {
                if (Platform.isLinux() && !Platform.is64Bits()) {
                    field.alignment = 4;
                } else {
                    field.alignment = 8;
                }
            }
        }
    }
}

However it results in an "Unaligned pointer access" exception from BridJ. I enabled SUPPORTS_UNALIGNED_ACCESS for all platforms in bridj.hpp and it now all works correctly but obviously I'm concerned about the impact of enabling SUPPORTS_UNALIGNED_ACCESS under Linux and also the inelegance of the customizer.

I would be happy to try and update the code in StructIO and generate a pull request to avoid the Customizer but that will still result in the Unaligned error.

Any help would be greatly appreciated.

ochafik added a commit that referenced this issue Jan 7, 2013

BridJ: Fixed alignment of double fields in structs on Linux 32 bits +…
… added BRIDJ_ALIGN_DOUBLE / bridj.alignDouble option (issue #320)
@ochafik

This comment has been minimized.

Show comment
Hide comment
@ochafik

ochafik Jan 7, 2013

Member

Hi @hughperkins , @estw ,

This should be fixed in the next release (0.6.2), which I'm unfortunately taking an unusually long time to perform (I'm still waiting for the renewal of my code signing certificates to publish to Maven Central :-S).

Will let you know here when it's available, in the meanwhile please build from sources.

Cheers

Member

ochafik commented Jan 7, 2013

Hi @hughperkins , @estw ,

This should be fixed in the next release (0.6.2), which I'm unfortunately taking an unusually long time to perform (I'm still waiting for the renewal of my code signing certificates to publish to Maven Central :-S).

Will let you know here when it's available, in the meanwhile please build from sources.

Cheers

@ochafik ochafik closed this Jan 7, 2013

@estw

This comment has been minimized.

Show comment
Hide comment
@estw

estw Jan 8, 2013

Hi @ochafik

Thanks, that's great. I have tested it and it has resolved the need for the Customizer however with the latest source it still results in the "Unaligned pointer access !" error i.e.

java.lang.RuntimeException: Unaligned pointer access !
    at org.bridj.JNI.get_double(Native Method) ~[bridj-0.6.3-SNAPSHOT.jar:na]
    at org.bridj.Pointer$OrderedPointer.getDoubleAtOffset(Pointer.java:484) ~[bridj-0.6.3-SNAPSHOT.jar:na]
    at org.bridj.StructIO.getDoubleField(StructIO.java:1003) ~[bridj-0.6.3-SNAPSHOT.jar:na]

This appears to work by defining SUPPORTS_UNALIGNED_ACCESS in BridJ/src/main/cpp/bridj/bridj.hpp for Linux however I'm not sure if that's the right solution?

Thanks again.

estw commented Jan 8, 2013

Hi @ochafik

Thanks, that's great. I have tested it and it has resolved the need for the Customizer however with the latest source it still results in the "Unaligned pointer access !" error i.e.

java.lang.RuntimeException: Unaligned pointer access !
    at org.bridj.JNI.get_double(Native Method) ~[bridj-0.6.3-SNAPSHOT.jar:na]
    at org.bridj.Pointer$OrderedPointer.getDoubleAtOffset(Pointer.java:484) ~[bridj-0.6.3-SNAPSHOT.jar:na]
    at org.bridj.StructIO.getDoubleField(StructIO.java:1003) ~[bridj-0.6.3-SNAPSHOT.jar:na]

This appears to work by defining SUPPORTS_UNALIGNED_ACCESS in BridJ/src/main/cpp/bridj/bridj.hpp for Linux however I'm not sure if that's the right solution?

Thanks again.

@ochafik

This comment has been minimized.

Show comment
Hide comment
@ochafik

ochafik Jan 8, 2013

Member

Ouch, sorry about that one! Pointer-related code must indeed let this half-alignment pass through properly.
Reopening this issue.

Member

ochafik commented Jan 8, 2013

Ouch, sorry about that one! Pointer-related code must indeed let this half-alignment pass through properly.
Reopening this issue.

@ochafik

This comment has been minimized.

Show comment
Hide comment
@ochafik

ochafik Jun 9, 2013

Member

Sorry about the huge delay!
The latest 0.7-SNAPSHOT should contain a fix, please let me know of any issue!

Member

ochafik commented Jun 9, 2013

Sorry about the huge delay!
The latest 0.7-SNAPSHOT should contain a fix, please let me know of any issue!

@ochafik ochafik closed this Jun 9, 2013

@estw

This comment has been minimized.

Show comment
Hide comment
@estw

estw Jun 10, 2013

Great, thanks Olivier. I'll give it a run through this week and let you
know.

On Sun, Jun 9, 2013 at 8:18 PM, Olivier Chafik notifications@github.comwrote:

Sorry about the huge delay!
The latest 0.7-SNAPSHOT should contain a fix, please let me know of any
issue!


Reply to this email directly or view it on GitHubhttps://github.com//issues/320#issuecomment-19171667
.

estw commented Jun 10, 2013

Great, thanks Olivier. I'll give it a run through this week and let you
know.

On Sun, Jun 9, 2013 at 8:18 PM, Olivier Chafik notifications@github.comwrote:

Sorry about the huge delay!
The latest 0.7-SNAPSHOT should contain a fix, please let me know of any
issue!


Reply to this email directly or view it on GitHubhttps://github.com//issues/320#issuecomment-19171667
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment