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

Wrong size of c_<type> for 16 bit target #1125

Closed
vegecode opened this Issue Jun 17, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@vegecode

vegecode commented Jun 17, 2018

; Linker script provides the address for the WDTCTL register which
; then is used in the device specific header like so
;sfr_w(WDTCTL);                                /* Watchdog Timer Control */

; an iomacros.h file provides the macros below that declare a variable
; WDTCTL is declared as an unsigned int which on a 16 bit target I
; assumed would be 16 bits like it is in C
;#define sfr_b(x) extern volatile unsigned char x
;#define sfr_w(x) extern volatile unsigned int x
;#define sfr_a(x) extern volatile unsigned long int x
;#define sfr_l(x) extern volatile unsigned long int x

; Compiler is treating as 32 bit instead of 16 bit
main:
	push.w	r4
	mov.w	r1, r4
	mov.w	#0, &WDTCTL+2    ;<<<< This line should not be here.
	mov.w	#23168, &WDTCTL
	jmp	.LBB2_1
.LBB2_1:
	jmp	.LBB2_1

Changing the sfr_x definitions to char, short, and int respectively fix the problem for now.

In zig/Target.cpp in function uint32_t target_c_type_size_in_bits(const ZigTarget *target, CIntType id), the sizes are defined based on the operating system type. Would it make more sense for them to be based on the architecture?

By the current logic, I think that on the msp430 a long is shorter than an int. A long would be 16 bit and an int would be 32 bit.

Perhaps compiler switches to override the default would be easiest for supporting architectures that deviate from whatever the most common Zig platform is.

@andrewrk andrewrk added this to the 0.3.0 milestone Jun 17, 2018

@andrewrk andrewrk added the bug label Jun 17, 2018

@andrewrk

This comment has been minimized.

Member

andrewrk commented Jun 17, 2018

The target is composed of:

  • architecture (msp430 for you)
  • OS (freestanding for you)
  • C environment

Between these things, we can figure out what sizes the C types should be.

I found this in clang:

  MSP430TargetInfo(const llvm::Triple &Triple, const TargetOptions &)
      : TargetInfo(Triple) {
    TLSSupported = false;
    IntWidth = 16;
    IntAlign = 16;
    LongWidth = 32;
    LongLongWidth = 64;
    LongAlign = LongLongAlign = 16;
    PointerWidth = 16;
    PointerAlign = 16;
    SuitableAlign = 16;
    SizeType = UnsignedInt;
    IntMaxType = SignedLongLong;
    IntPtrType = SignedInt;
    PtrDiffType = SignedInt;
    SigAtomicType = SignedLong;

So I think we can simply add this information to Zig.

@andrewrk andrewrk closed this in e5956f2 Jun 17, 2018

@vegecode

This comment has been minimized.

vegecode commented Jun 17, 2018

In e5956f2, a long is still 16 bits. It should be 32 bits, I believe.

@andrewrk andrewrk reopened this Jun 17, 2018

@andrewrk

This comment has been minimized.

Member

andrewrk commented Jun 17, 2018

Hmm. Looks like we should hard code it to 32 bits rather than pointer size.

@vegecode

This comment has been minimized.

vegecode commented Jun 17, 2018

It appears so.

@andrewrk andrewrk closed this in 92a3604 Jun 18, 2018

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