Skip to content
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

Fix checkValidEnum outputting non-unique names when cViaASM is enabled #89

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jorisdral
Copy link

@jorisdral jorisdral commented Apr 4, 2024

I ran into a compiler failure when cross-compiling the following code to Windows via ASM.

https://github.com/input-output-hk/haskell-lmdb/blob/master/src/Database/LMDB/FFI.hsc#L159-L161

newtype MDB_cursor_op = MDB_cursor_op ( #type MDB_cursor_op )
  deriving (Show, Eq)
#{enum MDB_cursor_op, MDB_cursor_op, MDB_FIRST, MDB_FIRST_DUP, MDB_GET_BOTH, MDB_GET_BOTH_RANGE, MDB_GET_CURRENT, MDB_GET_MULTIPLE, MDB_LAST, MDB_LAST_DUP, MDB_NEXT, MDB_NEXT_DUP, MDB_NEXT_MULTIPLE, MDB_NEXT_NODUP, MDB_PREV, MDB_PREV_DUP, MDB_PREV_NODUP, MDB_SET, MDB_SET_KEY, MDB_SET_RANGE}

I saw 17 error: redefinition of... errors in the log, and since there are 18 values in the enum I was defining, this lead me to believe something in hsc2hs wasn't going right.

Log: compilation error
Configuring library 'ffi' for cardano-lmdb-0.4.0.0..
Warning: --enable-library-for-ghci is no longer supported on Windows with GHC
9.4 and later; ignoring...
building
Preprocessing library 'ffi' for cardano-lmdb-0.4.0.0..
FFI.hsc:163:11: error: redefinition of ‘_hsc2hs_test_noKey5’
FFI.hsc:163:11: note: previous definition of ‘_hsc2hs_test_noKey5’ with type ‘long long int’
FFI.hsc:163:11: error: redefinition of ‘_hsc2hs_test_noKey5’
FFI.hsc:163:11: note: previous definition of ‘_hsc2hs_test_noKey5’ with type ‘long long int’
FFI.hsc:163:11: error: redefinition of ‘_hsc2hs_test_noKey5’
FFI.hsc:163:11: note: previous definition of ‘_hsc2hs_test_noKey5’ with type ‘long long int’
FFI.hsc:163:11: error: redefinition of ‘_hsc2hs_test_noKey5’
FFI.hsc:163:11: note: previous definition of ‘_hsc2hs_test_noKey5’ with type ‘long long int’
FFI.hsc:163:11: error: redefinition of ‘_hsc2hs_test_noKey5’
FFI.hsc:163:11: note: previous definition of ‘_hsc2hs_test_noKey5’ with type ‘long long int’
FFI.hsc:163:11: error: redefinition of ‘_hsc2hs_test_noKey5’
FFI.hsc:163:11: note: previous definition of ‘_hsc2hs_test_noKey5’ with type ‘long long int’
FFI.hsc:163:11: error: redefinition of ‘_hsc2hs_test_noKey5’
FFI.hsc:163:11: note: previous definition of ‘_hsc2hs_test_noKey5’ with type ‘long long int’
FFI.hsc:163:11: error: redefinition of ‘_hsc2hs_test_noKey5’
FFI.hsc:163:11: note: previous definition of ‘_hsc2hs_test_noKey5’ with type ‘long long int’
FFI.hsc:163:11: error: redefinition of ‘_hsc2hs_test_noKey5’
FFI.hsc:163:11: note: previous definition of ‘_hsc2hs_test_noKey5’ with type ‘long long int’
FFI.hsc:163:11: error: redefinition of ‘_hsc2hs_test_noKey5’
FFI.hsc:163:11: note: previous definition of ‘_hsc2hs_test_noKey5’ with type ‘long long int’
FFI.hsc:163:11: error: redefinition of ‘_hsc2hs_test_noKey5’
FFI.hsc:163:11: note: previous definition of ‘_hsc2hs_test_noKey5’ with type ‘long long int’
FFI.hsc:163:11: error: redefinition of ‘_hsc2hs_test_noKey5’
FFI.hsc:163:11: note: previous definition of ‘_hsc2hs_test_noKey5’ with type ‘long long int’
FFI.hsc:163:11: error: redefinition of ‘_hsc2hs_test_noKey5’
FFI.hsc:163:11: note: previous definition of ‘_hsc2hs_test_noKey5’ with type ‘long long int’
FFI.hsc:163:11: error: redefinition of ‘_hsc2hs_test_noKey5’
FFI.hsc:163:11: note: previous definition of ‘_hsc2hs_test_noKey5’ with type ‘long long int’
FFI.hsc:163:11: error: redefinition of ‘_hsc2hs_test_noKey5’
FFI.hsc:163:11: note: previous definition of ‘_hsc2hs_test_noKey5’ with type ‘long long int’
FFI.hsc:163:11: error: redefinition of ‘_hsc2hs_test_noKey5’
FFI.hsc:163:11: note: previous definition of ‘_hsc2hs_test_noKey5’ with type ‘long long int’
FFI.hsc:163:11: error: redefinition of ‘_hsc2hs_test_noKey5’
FFI.hsc:163:11: note: previous definition of ‘_hsc2hs_test_noKey5’ with type ‘long long int’
compilation failed

After a little bit of digging, I found the problem is in checkValidEnum, and in particular lines 309 and 310. Since in my example, I don't any define Haskell names for the values of the enum, hName in line 310 is Nothing, and so the name we pass to validConstTestViaAsm is always going to be noKey, and since none of the other parameters to validConstTestViaAsm change, it will print the same line each time.

hsc2hs/src/CrossCodegen.hs

Lines 306 to 319 in a6d9f73

checkValidEnum =
case parseEnum value of
Nothing -> ""
Just (_,_,enums) | viaAsm ->
concatMap (\(hName,cName) -> validConstTestViaAsm (fromMaybe "noKey" (ATT.trim `fmap` hName) ++ show uniq) cName) enums
Just (_,_,enums) ->
"void _hsc2hs_test" ++ show uniq ++ "()\n{\n" ++
concatMap (\(_,cName) -> validConstTest cName) enums ++
"}\n"
-- we want this to fail if the value is syntactically invalid or isn't a constant
validConstTest value' = outCLine pos ++ " {\n static int test_array[(" ++ value' ++ ") > 0 ? 2 : 1];\n (void)test_array;\n }\n"
validConstTestViaAsm name value' = outCLine pos ++ "\nextern long long _hsc2hs_test_" ++ name ++";\n"
++ "long long _hsc2hs_test_" ++ name ++ " = (" ++ value' ++ ");\n"

The solution I'm proposing here is to use the (trimmed) cName when we have no hName in scope. I'm not familiar with this code base, so let me know if this PR is okay as is.

My temporary local "solution" is to just give Haskell names to each value of the enum.

@jorisdral jorisdral force-pushed the jdral/fix-checkvalidenum-asm branch from a428a8c to e557e41 Compare April 4, 2024 16:43
jorisdral added a commit to input-output-hk/haskell-lmdb that referenced this pull request Apr 5, 2024
This is a workaround for
haskell/hsc2hs#89, because
not having explicit Haskell names prevents
cross-compilation via ASM (for now).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant