Keymap extras affecting Windows 7 users / comments affecting code size

[jackhumbert]

There's two parts to this problem, and I'm hoping those more experienced with C compilers can shed some light on the first:

Commenting out #includes doesn't decrease the file size in the same way that deleting the lines does

Below is the code that you can add to a keymap.c, and will increase the file size by 12 bytes:

//#include "keymap_fr_ch.h"
//#include "keymap_french.h"
//#include "keymap_german.h"
//#include "keymap_german_ch.h"
//#include "keymap_nordic.h"
//#include "keymap_norwegian.h"
//#include "keymap_spanish.h"

The filesize stays the same if you uncomment them. My understanding of this is that comments shouldn't affect code size. Is this some sort of compiler setting we're not paying attention to?

Including some of these keymap extras causes Windows 7 to not recognise the keyboard

@ezuk and I have been trying to figure this out for a while, and determined this was cause of the problems. My guess was that one of these undefines/redefines something at a system level, but I've been unable to find anything that might be affected by this.

Any insight would be helpful!

[skullydazed]

AFAIK the pre-processor should remove comments before expanding #includes. Do you get the same result if you put #if 0 #endif around it?

It would be interesting to add -E to the GCC line and see what the output looks like in each of the situations (uncommented, commented, removed, and #if 0'd.)

[skullydazed]

Looking closer, I'm not able to reproduce. Am I doing the right thing?

zach-desktop:planck zwhite$ git status
On branch master
Your branch is up-to-date with 'origin/master'.
zach-desktop:planck zwhite$ git pull
Already up-to-date.
zach-desktop:planck zwhite$ make default
...
Size after:
   text    data     bss     dec     hex filename
      0   27646       0   27646    6bfe planck_rev4_default.hex

zach-desktop:planck zwhite$ vi keymaps/default/keymap.c
zach-desktop:planck zwhite$ git diff
diff --git a/keyboards/planck/keymaps/default/keymap.c b/keyboards/planck/keymaps/default/keymap.c
index ddbe4d7..bf1f1ba 100644
--- a/keyboards/planck/keymaps/default/keymap.c
+++ b/keyboards/planck/keymaps/default/keymap.c
@@ -1,6 +1,14 @@
 // This is the canonical layout file for the Quantum project. If you want to add another keyboard,
 // this is the style you want to emulate.

+//#include "keymap_fr_ch.h"
+//#include "keymap_french.h"
+//#include "keymap_german.h"
+//#include "keymap_german_ch.h"
+//#include "keymap_nordic.h"
+//#include "keymap_norwegian.h"
+//#include "keymap_spanish.h"
+
 #include "planck.h"
 #include "action_layer.h"
 #ifdef AUDIO_ENABLE
zach-desktop:planck zwhite$ make default
...
Size after:
   text    data     bss     dec     hex filename
      0   27646       0   27646    6bfe planck_rev4_default.hex

[jackhumbert]

We were trying it exclusively with the default ErgoDox EZ layout, make ergodox-default - I'll mess around with those options now.

[skullydazed]

Curiouser and curiouser. I do reproduce it on ez:

default      0    22834       0   22834    5932 ergodox_ez_default.hex
comment      0    22846       0   22846    593e ergodox_ez_default.hex

[jackhumbert]

#if 0 doesn't change the size for me. I tried cleaning (make ergodox-default-clean) and then it gave me a No such file error on the .elf file with the -E option. There's a bunch of warning output about the linking (linker input file unused because linking not done) in all cases, but I'm not sure it's relevant.

[skullydazed]

The linker error makes sense, since the files will have C code instead of objects, so something couldn't build. Set the keymap.o file aside and repeat the process, then compare them.

[jackhumbert]

Ah, gotcha. Here's the diff between just commenting and uncommenting those lines:

 diff ergodox_keymap_uncommented.o ergodox_keymap_commented.o
6396,6425c6396
<
< # 1 "./quantum/keymap_extras/keymap_fr_ch.h" 1
<
<
<
< # 1 "./quantum/keymap.h" 1
< # 5 "./quantum/keymap_extras/keymap_fr_ch.h" 2
< # 7 "keyboards/ergodox/keymaps/default/keymap.c" 2
< # 1 "./quantum/keymap_extras/keymap_french.h" 1
< # 8 "keyboards/ergodox/keymaps/default/keymap.c" 2
< # 1 "./quantum/keymap_extras/keymap_german.h" 1
< # 9 "keyboards/ergodox/keymaps/default/keymap.c" 2
< # 1 "./quantum/keymap_extras/keymap_german_ch.h" 1
< # 10 "keyboards/ergodox/keymaps/default/keymap.c" 2
< # 1 "./quantum/keymap_extras/keymap_nordic.h" 1
< # 11 "keyboards/ergodox/keymaps/default/keymap.c" 2
< # 1 "./quantum/keymap_extras/keymap_norwegian.h" 1
<
<
<
< # 1 "./quantum/keymap_extras/keymap_nordic.h" 1
< # 5 "./quantum/keymap_extras/keymap_norwegian.h" 2
< # 12 "keyboards/ergodox/keymaps/default/keymap.c" 2
< # 1 "./quantum/keymap_extras/keymap_spanish.h" 1
< # 13 "keyboards/ergodox/keymaps/default/keymap.c" 2
<
<
<
<
<
---
> # 18 "keyboards/ergodox/keymaps/default/keymap.c"

[jackhumbert]

That's with the -E flag - without it, the keymap.o files are the same (not sure if that's expected).

[skullydazed]

I have two different crotchety C programmers scratching their heads over this. They're just as confused.

What's the magic invocation to show the gcc commands?

[jackhumbert]

I'm not sure of the command, but all of the options are here.

#---------------- Compiler Options C ----------------
#  -g*:          generate debugging information
#  -O*:          optimization level
#  -f...:        tuning, see GCC manual and avr-libc documentation
#  -Wall...:     warning level
#  -Wa,...:      tell GCC to pass this to the assembler.
#    -adhlns...: create assembler listing
CFLAGS += -g$(DEBUG)
CFLAGS += $(CDEFS)
CFLAGS += -O$(OPT)
# add color
ifeq ($(COLOR),true)
ifeq ("$(shell echo "int main(){}" | $(CC) -fdiagnostics-color -x c - -o /dev/null 2>&1)", "")
    CFLAGS+= -fdiagnostics-color
endif
endif
CFLAGS += -Wall
CFLAGS += -Wstrict-prototypes
#CFLAGS += -mshort-calls
#CFLAGS += -fno-unit-at-a-time
#CFLAGS += -Wundef
#CFLAGS += -Wunreachable-code
#CFLAGS += -Wsign-compare
CFLAGS += -Wa,-adhlns=$(@:%.o=%.lst)
CFLAGS += $(CSTANDARD)

Earlier in the same file:

CSTANDARD = -std=gnu99

[skullydazed]

After a bit of reverse engineering I figured out where I needed to remove the @ to find this:

avr-gcc -c -mmcu=atmega32u4 -funsigned-char -funsigned-bitfields -ffunction-sections -fdata-sections -fpack-struct -fshort-enums -E -fno-inline-small-functions -fno-strict-aliasing -gdwarf-2  -Os -Wall -Wstrict-prototypes -Wa,-adhlns=.build/obj_ergodox_ez_default/keyboards/ergodox/keymaps/default/keymap.lst -std=gnu99  -DSUBPROJECT_ez -DINTERRUPT_CONTROL_ENDPOINT -DBOOTLOADER_SIZE=512 -DUNICODE_ENABLE -DRGBLIGHT_ENABLE -DMAGIC_ENABLE -DMOUSEKEY_ENABLE -DMOUSE_ENABLE -DEXTRAKEY_ENABLE -DNO_PRINT -DNO_DEBUG -DCOMMAND_ENABLE -DNKRO_ENABLE -DONEHAND_ENABLE -DF_USB=16000000UL -DARCH=ARCH_AVR8 -DUSB_DEVICE_ONLY -DUSE_FLASH_DESCRIPTORS -DUSE_STATIC_OPTIONS="(USB_DEVICE_OPT_FULLSPEED | USB_OPT_REG_ENABLED | USB_OPT_AUTO_PLL)" -DFIXED_CONTROL_ENDPOINT_SIZE=8  -DFIXED_NUM_CONFIGURATIONS=1 -DPROTOCOL_LUFA -DF_CPU=16000000UL -DQMK_KEYBOARD=\"ergodox\" -DQMK_KEYMAP=\"default\"  -Ikeyboards/ergodox/keymaps/default -Ikeyboards/ergodox/ez -Ikeyboards/ergodox -I. -I./tmk_core -I./quantum -I./quantum/keymap_extras -I./quantum/audio -I./quantum/process_keycode -I./quantum/api -I./quantum/serial_link -Itmk_core/protocol -I./tmk_core/common -I./tmk_core/protocol/lufa -I./tmk_core/protocol/lufa/LUFA-git -include keyboards/ergodox/ez/config.h -MMD -MP -MF .build/obj_ergodox_ez_default/keyboards/ergodox/keymaps/default/keymap.td keyboards/ergodox/keymaps/default/keymap.c -o .build/obj_ergodox_ez_default/keyboards/ergodox/keymaps/default/keymap.o

I'm still going through these to figure out if anything here could be the culprit.

[skullydazed]

OK, so I have more information but I don't know if we have anything conclusive yet. Here's a basic theory about why you're seeing 12 extra bytes.

One of the flags that is passed is -gdwarf-2. This turns on debugging information, and thus line number reporting. When examining the two hexes it looks like a lot of the changes trace back to an elf section named .debug_line. So when you add the 6 commented out lines you also increase the number reported for every line after that. It looks like perhaps .debug_line gets stripped out somewhere along the way from elf to bin to hex. That process could explain the extra 12 bytes.

[skullydazed]

The information above came from @pablomarx. He generously spent some decompiling and examining the two .hex files to see what differences there were and here are his findings. Unfortunately he's not familiar with the QMK codebase so there's a lot he doesn't have the context to understand.

pablo_ there were four instructions in the "stabstr" function that changed subtly pablo_ it looks like dealing with a difference in a number pablo_ < 34: 63 67 ori r22, 0x73 ; 115
pablo_ < 36: 4a 7a andi r20, 0xAA ; 170
pablo_ < 38: 41 4b sbci r20, 0xB1 ; 177
pablo_ < 3a: 53 2e mov r5, r19
pablo_ ---
pablo_ > 34: 63 6c ori r22, 0xC3 ; 195
pablo_ > 36: 5a 78 andi r21, 0x8A ; 138
pablo_ > 38: 4f 44 sbci r20, 0x4F ; 79
pablo_ > 3a: 68 2e mov r6, r24
pablo_ The assembler creates two custom sections, a section named .stab' which contains an array of fixed length structures, one struct per stab, and a section named.stabstr' containing all the variable length strings that are referenced by stabs in the `.stab' section pablo_ cgJzAKS. vs clZxODh. pablo_ if those were to be interpreted as ascii instead of avr instructions pablo_ and guess what pablo_ /var/folders/qw/2dzm7tkn4ldf1xmn64kb6myr0000gn /T//ccgJzAKS.s
pablo_ from one elf pablo_ /var/folders/qw/2dzm7tkn4ldf1xmn64kb6myr0000gn /T//cclZxODh.s
pablo_ from the other pablo_ version.h also has changing data in it pablo_ $(shell echo '#define QMK_VERSION "$(GIT_VERSION)"' > $(ROOTDIR)/quantum/version.h)
*pablo $(shell echo '#define QMK_BUILDDATE "$(BUILD_DATE)"' >> $(ROOT_DIR)/quantum/version.h)
pablo which seems to be used all over the place in keymaps pablo e.g. pablo_ keyboards/ergodox/keymaps/reset_eeprom/keymap.c
pablo_* 73: SEND_STRING (QMK_KEYBOARD "/" QMK_KEYMAP " @ " QMK_VERSION);

[skullydazed]

Ok, we know where the extra 12 bytes come from, and I don't think it is causing the issue you see on windows 7. When those lines are not in place this is the version string:

ergodox/default @ ergodox_ez-161205-265-g841d7e

When you put them in in a commented form this is the version string:

ergodox/default @ ergodox_ez-161205-265-g841d7e-dirty

The version string occurs twice in the compiled hex, 6 extra characters in the latter instance, which equals 12 bytes.

Thanks to @pablomarx for finding that.

That still leaves the problem of why including some of those features breaks windows 7. Could it be a unicode input thing?

[jackhumbert]

Oh geez - that's ridiculous :) Thanks for figuring this out! That explains why adding it to the Planck layout didn't do anything - I'm not using that in the default like the EZ is.

[keaston]

...and of course the -dirty isn't directly because of the commented-out lines - it's because you did that as an uncommitted change. If you committed that to git, the -dirty should disappear and the size go back down.

[fredizzimo]

That's interesting.

The problem could probably have been spotted more easily if you just do a diff between the two generated map files. They contain all the addresses and sizes of all symbols, so the first line that is different should probably indicate the issue.

However there's two additional things. First it's probably not a good idea to define the strings in the version.h file, as they are then sometimes included several times as demonstrated here. So therefore they should be moved to the C code in version.c and declared as extern symbols. The version string alone (without dirty) is 48 bytes and the date is 20 bytes, so if the string is included twice, that's 68 bytes wasted memory. The make file should also be changed so that version.c is only compiled and included if the version string is actually needed, otherwise it will increase the compile times for all keyboards, but the symbols would probably be stripped out in either case.

We also need to make sure that they are stored in the program memory and not in the ram, 68 bytes of 2.5K on Atmega32u4 is quite much. That should already be the case if the SEND_STRING macro is used. But nevertheless if it's moved to version.c care should be taken.

[drashna]

I think that it's safe to close this now?