config: Set preprocessor defaults for ECMULT_* config values #1121

pull real-or-random wants to merge 3 commits into bitcoin-core:master from real-or-random:202207-config changing 5 files +28 −18
  1. real-or-random commented at 1:37 pm on July 6, 2022: contributor

    This simplifies manual builds and solves one item in #929.

    Manual compilation now:

    0gcc -O2 -c src/secp256k1.c src/precomputed_*.c
1ar rcs libsecp256k1.a secp256k1.o precomputed_*.o

    The second commit will make debugging the config easier when passing -DCONFIG_DEBUG. It currently only prints the ECMULT_* variables but we could use it for WIDEMUL and also synthetic int128 stuff in the future after #1000 is merge. The logic there is more complex than simply falling back to a default value.

    But I’m happy to drop that commit if people don’t like this or think it’s overkill.

    edit: The third commit removes basic-config.h which is now definitively not necessary anymore because we have default values.

    My long-term plan is to get rid of the config header “src/libsecp256k1-config.h” currently generated by autotools and simply let autotools pass all config macros via -D switches. Generated header files are annoying, and could then also be avoided in #1113. See #1113 (comment) for more context.

  2. config: Set preprocessor defaults for ECMULT_* config values 
    This simplifies manual builds and solves one item in #929.
    d0cf55e13a
  3. real-or-random force-pushed on Jul 6, 2022
  4. real-or-random cross-referenced this on Jul 6, 2022 from issue [secp256k1] Update secp256k1 from 2017 to 2022 by R0g3r10LL31t3
  5. sipa commented at 2:29 pm on July 6, 2022: contributor

    Concept ACK, and also agree with the intent to get rid of the config files entirely.

    One thing I’d like to bring up though: I think it may be useful to have a strict separation between “external” macros (provided in config.h, or through compiler -D flags) flags, and “internal” macros which guide the actual source code conditionals? I can imagine having a single file that does all conversion between the two (and e.g. contains all logic for autodetecting endianness, integer sizes, plus looks at external variables for overriding/guiding them). A downside would be that this file ends up containing logic relating to many aspects of the codebase, while those decisions are now closer to where they’re actually used, but having everything together also gives a much better overview.

    If we go that direction, it doesn’t need to happen now of course, but I wanted to bring up in case we want to start working in that direction already.

  6. real-or-random commented at 3:55 pm on July 6, 2022: contributor

    I think it may be useful to have a strict separation between “external” macros (provided in config.h, or through compiler -D flags) flags, and “internal” macros which guide the actual source code conditionals?

    Yeah, I was considering this too. A simple way may be to have a prefix for all external/input macros, e.g., CONFIG_. (We could still accept the non-prefixed form for backwards-compability).

    I can imagine having a single file that does all conversion between the two (and e.g. contains all logic for autodetecting endianness, integer sizes, plus looks at external variables for overriding/guiding them). A downside would be that this file ends up containing logic relating to many aspects of the codebase, while those decisions are now closer to where they’re actually used, but having everything together also gives a much better overview.

    Indeed. I think this is basically an incarnation of the old debate whether you’d want a single config object / singleton, which in our case would give a better overview but less locality. I don’t think there’s an absolute answer but my current thinking is that we’ll anyway have at least one other place that will have nice overview over all options (e.g., the README, or configure --help), so the “better overview” point is somewhat useless here. So I’d prefer to keep the preprocessor “logic” local to the respective modules and files. Note that this is not always possible, e.g., the WIDEMUL macros influence a whole bunch of other files. But there’s nothing wrong with keeping them in util.h (where we keep other stuff that is used across various files).

  7. sipa commented at 1:53 pm on July 7, 2022: contributor

    A simple way may be to have a prefix for all external/input macros, e.g., CONFIG_. (We could still accept the non-prefixed form for backwards-compability).

    Do you mean CONFIG_ as prefix for the internal ones, or the external ones? In either case, perhaps you don’t want to use the names CONFIG_DEBUG, CONFIG_DEBUG_MSG, CONFIG_DEBUG_DEF in this PR, as one of those is external and the two others are internal?

  8. real-or-random commented at 2:49 pm on July 7, 2022: contributor

    A simple way may be to have a prefix for all external/input macros, e.g., CONFIG_. (We could still accept the non-prefixed form for backwards-compability).

    Do you mean CONFIG_ as prefix for the internal ones, or the external ones?

    I meant CONFIG for the external ones, because this tells the user “if it starts with CONFIG, I’m allowed to set it”.

    In either case, perhaps you don’t want to use the names CONFIG_DEBUG, CONFIG_DEBUG_MSG, CONFIG_DEBUG_DEF in this PR, as one of those is external and the two others are internal?

    That’s a very good point… I’ll change it.

  9. sipa commented at 2:51 pm on July 7, 2022: contributor

    I meant CONFIG for the external ones, because this tells the user “if it starts with CONFIG, I’m allowed to set it”.

    SGTM

  10. sipa commented at 4:51 pm on July 7, 2022: contributor
    Would it make sense to have the external config options be prefixed by SECP256K1_ or something similar? That would scope them clearly, when libsecp256k1 is being compiled by being directly included in another project.
  11. hebasto commented at 5:44 pm on July 7, 2022: member
    Concept ACK.
  12. config: Introduce DEBUG_CONFIG macro for debug output of config da6514a04a
  13. config: Remove basic-config.h 
    It's unused and thus potentially confusing.
    c27ae45144
  14. real-or-random force-pushed on Jul 7, 2022
  15. real-or-random commented at 6:42 pm on July 7, 2022: contributor

    Do you mean CONFIG_ as prefix for the internal ones, or the external ones? In either case, perhaps you don’t want to use the names CONFIG_DEBUG, CONFIG_DEBUG_MSG, CONFIG_DEBUG_DEF in this PR, as one of those is external and the two others are internal?

    In interest of pushing to bikeshedding to the PR that actually introduces the prefix, I simply replaced the string CONFIG_DEBUG by DEBUG_CONFIG. This leaves the CONFIG_ prefix free for future use if we want it, e.g., for CONFIG_DEBUG_CONFIG.

    Maybe this was not what you intended, feel free to complain. But I felt that making a distinction between internal and external for the macros introduced here is not worth much: Anyway a future PR will need to do a lot renaming. Currently we for example have ECMULT_GEN_PREC_BITS (external) and ECMULT_GEN_PREC_G (internal). Not to mention that names start with E are reserved and defining a macro ECMULT_GEN_PREC_BITS is UB (in a very pedantic reading of the standard).

    That would scope them clearly, when libsecp256k1 is being compiled by being directly included in another project.

    Do you think that’s a use case we should support?

  16. sipa commented at 6:49 pm on July 7, 2022: contributor

    In interest of pushing to bikeshedding to the PR that actually introduces the prefix

    Yeah, it shouldn’t hold this up if more discussion is needed.

    Do you think that’s a use case we should support?

    Possibly, but let’s keep that for elsewhere.

  17. sipa commented at 6:55 pm on July 7, 2022: contributor

    ACK c27ae451440bdaf68bf8aaa60edb1f4b4614d492

    Tested building without config: gcc -fPIC -DDEBUG_CONFIG -O3 -Wall -Wno-unused -march=native -std=c89 src/secp256k1.c src/precomputed_ecmult.c src/precomputed_ecmult_gen.c -shared -o sec.so.

  18. hebasto approved
  19. hebasto commented at 11:31 am on July 8, 2022: member

    ACK c27ae451440bdaf68bf8aaa60edb1f4b4614d492, I have reviewed the code and it looks correct.

    Tested with ./configure CPPFLAGS=-DDEBUG_CONFIG.

    However,

    The second commit will make debugging the config easier when passing -DCONFIG_DEBUG. It currently only prints the ECMULT_* variables but we could use it for WIDEMUL and also synthetic int128 stuff in the future after #1000 is merge. The logic there is more complex than simply falling back to a default value.

    Even now debug messages are noisy as they repeat for every translation unit which includes a relevant header directly or indirectly. Adding more macros will make build logs messier. This approach does not look scalable to me.

  20. sipa commented at 1:51 pm on July 8, 2022: contributor

    Even now debug messages are noisy as they repeat for every translation unit which includes a relevant header directly or indirectly. Adding more macros will make build logs messier. This approach does not look scalable to me.

    I don’t think that’s a concern. You don’t need to use it, as it’s just for helping debug configuration issues, and isn’t enabled by default. How does scalability matter? Do you expect more macros like this?

  21. hebasto commented at 2:00 pm on July 8, 2022: member

    Do you expect more macros like this?

    It currently only prints the ECMULT_* variables but we could use it for WIDEMUL and also synthetic int128 stuff in the future after #1000 is merge.

  22. sipa commented at 2:01 pm on July 8, 2022: contributor
    @hebasto That’s about extended the output to more configuration macros, not adding more macros. I may be misunderstanding what you’re talking about.
  23. hebasto commented at 2:03 pm on July 8, 2022: member

    That’s about extended the output to more configuration macros…

    Yes, exactly. Sorry for not being clear.

  24. real-or-random commented at 4:17 pm on July 8, 2022: contributor

    I don’t think that’s a concern. You don’t need to use it, as it’s just for helping debug configuration issues, and isn’t enabled by default. How does scalability matter? Do you expect more macros like this?

    Yep I agree. It’s noisy but the point is to be noisy when we ask for it. I don’t think build log clutter is an issue. I see this as an option for developers and maintainers. You’ll only enable this if you want to debug the config decisions, and yes, in that case, you may have to look at a wall of text if you use a build system. But I guess it’s fine.

  25. jonasnick commented at 12:06 pm on July 11, 2022: contributor

    ACK c27ae451440bdaf68bf8aaa60edb1f4b4614d492

    Tested various configurations. I didn’t find the duplicate output of DEBUG_CONFIG too confusing; it’s only a debug tool after all.

  26. jonasnick merged this on Jul 11, 2022
  27. jonasnick closed this on Jul 11, 2022
  28. dhruv referenced this in commit f70e7d8108 on Jul 20, 2022
  29. hebasto referenced this in commit 07695f6c2d on Jul 21, 2022
  30. dhruv referenced this in commit e5166959a4 on Jul 21, 2022
  31. dhruv referenced this in commit 726cbfe06c on Jul 21, 2022
  32. dhruv referenced this in commit c354ccd3e6 on Jul 21, 2022
  33. dhruv referenced this in commit 296cb3807d on Jul 21, 2022
  34. dhruv referenced this in commit a7efff1c21 on Jul 22, 2022
  35. dhruv referenced this in commit 5667aa958a on Aug 12, 2022
  36. dhruv referenced this in commit 06823cfe29 on Aug 24, 2022
  37. real-or-random cross-referenced this on Aug 25, 2022 from issue build: Add CMake-based build system by hebasto
  38. dhruv referenced this in commit 6eca30d4bd on Sep 2, 2022
  39. dhruv referenced this in commit c3ed192dda on Sep 2, 2022
  40. dhruv referenced this in commit 89ebab0601 on Oct 1, 2022
  41. dhruv referenced this in commit d6bcb105c3 on Oct 20, 2022
  42. dhruv referenced this in commit c27eb1e66a on Oct 20, 2022
  43. dhruv referenced this in commit a2e91d2816 on Oct 20, 2022
  44. dhruv referenced this in commit 0b21533c10 on Oct 21, 2022
  45. dhruv referenced this in commit 01dddb4cf6 on Oct 21, 2022
  46. dhruv referenced this in commit a0bb5b6946 on Nov 17, 2022
  47. dhruv referenced this in commit 2e4c03dd67 on Nov 17, 2022
  48. dhruv referenced this in commit 388c9b1b55 on Nov 21, 2022
  49. dhruv referenced this in commit 244eb87643 on Dec 7, 2022
  50. dhruv referenced this in commit 92cddabc43 on Dec 8, 2022
  51. sipa referenced this in commit 9d47e7b71b on Dec 13, 2022
  52. dhruv referenced this in commit 55ffd47cc6 on Dec 14, 2022
  53. dhruv referenced this in commit 967c65b158 on Dec 14, 2022
  54. dhruv referenced this in commit 78b5ddf28b on Jan 11, 2023
  55. dhruv referenced this in commit 215394a1d5 on Jan 11, 2023
  56. div72 referenced this in commit 945b094575 on Mar 14, 2023
  57. str4d referenced this in commit 0df7b459f6 on Apr 21, 2023
  58. vmta referenced this in commit e1120c94a1 on Jun 4, 2023
  59. vmta referenced this in commit 8f03457eed on Jul 1, 2023
  60. jonasnick cross-referenced this on Jul 17, 2023 from issue Upstream PRs 1056, 1104, 1105, 1084, 1114, 1115, 1116, 1120, 1122, 1121, 1128, 1131, 1144, 1150, 1146 by jonasnick


real-or-random sipa hebasto jonasnick

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 01:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me