Fixed missing functions from doxygen headers #965

gilles-peskine-arm · 2017-06-15T16:50:51Z

Functions which had alternative implementations were omitted from the documentation generated by apidoc_full.sh, due to the way #define's were managed. This commit fixes this except for the ECC functions.

Internal ref: IOTSSL-1447. See also ARMmbed/mbed-os-5-docs#171

RonEld · 2017-06-22T12:23:04Z

Hi @gilles-peskine-arm the CI is failing because:

5.74s$ tests/scripts/doxygen.sh
/home/travis/build/ARMmbed/mbedtls/include/mbedtls/config.h:205: warning: documentation for unknown define MBEDTLS_TIMING_ALT found.
/home/travis/build/ARMmbed/mbedtls/include/mbedtls/config.h:218: warning: documentation for unknown define MBEDTLS_AES_ALT found.
/home/travis/build/ARMmbed/mbedtls/include/mbedtls/config.h:261: warning: documentation for unknown define MBEDTLS_MD2_PROCESS_ALT found.
/home/travis/build/ARMmbed/mbedtls/include/mbedtls/config.h:369: warning: documentation for unknown define MBEDTLS_ENTROPY_HARDWARE_ALT found.
/home/travis/build/ARMmbed/mbedtls/include/mbedtls/config.h:1332: warning: documentation for unknown define MBEDTLS_THREADING_ALT found.
FAIL

perhaps this post could assisst

gilles-peskine-arm · 2017-06-22T12:26:03Z

@RonEld Yes, I noticed that today. That SO link was the second Google hit for me, the first was #520 … I'm working on a solution, never mind if the way it's done isn't pretty as long as the output is good.

simonbutcher · 2017-10-01T12:05:28Z

Looks good to me. @Patater - if you can review this too, we can get it merged.

Patater · 2017-10-10T09:34:05Z

scripts/config.pl

@@ -96,7 +98,7 @@

 # Things that should be enabled in "full" even if they match @excluded
 my @non_excluded = qw(
-PLATFORM_[A-Z0-9]+_ALT
+PLATFORM_[_A-Z0-9]+_ALT


This now matches double (or more) _ like PLATFORM_____WHATEVER_STUFF_____ALT. Consider removing all _ from outside the [].

I personally don't think this matters. Whether the underscore is inside or outside the brackets, they'll still match to multiple underscores, but I don't think that's an issue.

Patater · 2017-10-10T09:38:20Z

scripts/config.pl

@@ -170,6 +172,9 @@
 if ($action eq "realfull") {
    $exclude_re = qr/^$/;
    $no_exclude_re = qr/./;
+} elsif ($action eq "docfull") {
+    $exclude_re = qr/_ALT\s*$/;
+    $no_exclude_re = qr/(?:PLATFORM|ECP)_[_A-Z0-9]+_ALT\s*$/;


This also matches double (or more) _. Consider removing all _ from outside the [].

This commit says it doesn't include ECC functions in doxygen output yet, but it perhaps is partially making the attempt by having ECP in this no exclude regex. Should the |ECP addition be in the ECC function no exclude commit instead?

I think the structure of the commits makes logical sense as they are. I don't see a need to restructure them.

Patater · 2017-10-10T09:40:14Z

doxygen/mbedtls.doxyfile

+                         md_internal.h \
+                         pk_internal.h \
+                         ssl_internal.h \
+                         *_wrap.h


Which _internal.h files are we additionally allowing now? Is it clear to folks adding new _internal.h files that they won't get excluded automatically anymore?

I think there has been a shift in what _internal.h is used for: it used to mean "don't include this in your application, it's only for internal library use", but in recently added occurrences it means "don't include this in your application, it's only for implementers of alternative crypto implementations". I wish we had distinct names for those two meanings but it's two late for that. Anyway, it looks like new _internal.h files will most likely fall in that second category, so should indeed be included by default. (Ideally we could have different doxyfiles for end-users and alt-implementers.)

I've added a comment to explain this.

Patater · 2017-10-10T09:45:07Z

include/mbedtls/ecp_internal.h

@@ -98,7 +104,7 @@ int mbedtls_internal_ecp_init( const mbedtls_ecp_group *grp );
 */
 void mbedtls_internal_ecp_free( const mbedtls_ecp_group *grp );

-#if defined(ECP_SHORTWEIERSTRASS)
+/* Short Weierstrass form */


I'm not really sure why removing these ECP_SHORTWEIERSTRASS and ECP_MONTGOMERY ifdefs causes ECP functions with ALT implementations to be included in the Doxygen output. If why isn't obvious to an informed developer, could you please explain why in the commit message?

I believe this will also fix #1147

I've added a clarifying description to the commit. Please let me know if it's sufficient.

Patater · 2017-10-10T11:36:06Z

ChangeLog

@@ -36,6 +36,8 @@ Changes
   * Clarify ECDSA documentation and improve the sample code to avoid
     misunderstandings and potentially dangerous use of the API. Pointed out
     by Jean-Philippe Aumasson.
+   * Tweaked apidoc_full.sh to include functions with alternative


What tense are we supposed to use in the ChangeLog?

We should use a consistent tense.

Judging from the surrounding ChangeLog entries, this should be Tweak for uniformity.

I've changed the tense and tried to improve the comment.

gilles-peskine-arm · 2017-11-27T19:25:27Z

@Patater I'm treating your comments here as “changes requested”, I'll take a look. @mpg I'd appreciate your opinion as to whether this upholds the philosophy of what should be included in the Doxygen output.

Patater · 2017-11-28T10:20:51Z

@gilles-peskine-arm The _ stuff is just "comment", not strictly requiring a change. With or without the _ outside it'll still match multiple ______.

The comment about tense should be addressed. The ECP regex falling into the wrong commit should be addressed as well.

mpg · 2017-11-30T12:54:37Z

@gilles-peskine-arm I think the philosophy of what goes to doxigen output is yet to be defined, so concerns about upholding it should not block a PR. Anyway, I believe this PR keeps the spirit of what we'd been doing previously.

mpg

This looks good to me, both the intent and how it's implemented.

However, there are non-trivial merge conflicts in config.pl which need to be addressed before merging.

Patater

We've merged platform_zeroize, so MBEDTLS_PLATFORM_ZEROIZE_ALT Doxygen isn't being generated properly. I'm happy to review after a rebase on latest development.

hanno-becker · 2018-08-24T14:26:04Z

@gilles-peskine-arm If you find time, could you rebase this?

gilles-peskine-arm · 2018-08-24T16:53:20Z

@hanno-arm Looking at the comments here there's more work to do than just rebasing. I won't have time until the week after next.

RonEld · 2018-10-25T11:48:55Z

fixes #520 , but the script for generating the documentation in the website should probably change as well

simonbutcher · 2019-01-09T22:07:11Z

@gilles-peskine-arm - Do you think you'll be able to rework this PR? Or can I adopt it (or find a volunteer)?

gilles-peskine-arm · 2019-01-09T23:35:35Z

This PR has bitrotted and I don't intend to work on it in the short or medium term.

Functions which had alternative implementations were omitted from the documentation generated by apidoc_full.sh, due to the way #define's were managed. This commit fixes this except for the ECC functions.

Some internal ECP functions which can be provided as alternative implementations to accelerate ECC operations were not being included in the doxygen generated documentation. This is because they were only being included in the header if the preprocessor symbols ECP_SHORTWEIERSTRASS and ECP_MONTGOMERY were included. Yet neither symbol is a config.h option, and were actually symbols that were defined internally in ecp.c, only after ecp_internal.h had been included. The missing functions also have no implementation in the library and are only defined as a means to provide acceleration through 3rd party implementation. Surrounding the functions by the symbols ECP_SHORTWEIERSTRASS and ECP_MONTGOMERY therefore is unnecessary, and stops the documentation from including them. This commit therefore removes those guards. Fixes Mbed-TLS#1147.

Fix the tense in the ChangeLog and refine the language.

Added a comment to doxygen/mbedtls.doxyfile to explain what is and isn't excluded from doxygen documentation and why.

simonbutcher · 2019-01-12T00:27:35Z

I've rebased the PR, and attempted to address review feedback.

@RonEld / @Patater / @mpg - Could you please review? Once it's been reviewed I'll do the backports.

gilles-peskine-arm · 2020-11-14T11:22:14Z

scripts/apidoc_full.sh

@@ -19,7 +19,7 @@ fi
 CONFIG_BAK=${CONFIG_H}.bak
 cp -p $CONFIG_H $CONFIG_BAK

-scripts/config.pl realfull
+scripts/config.pl docfull


Since we only use realfull to generate the documentation (which was its original purpose in 1989caf), we don't document it, and the library doesn't even build (check_config.h would signal conflicts), I think we should change what realfull does to be useful for its intended purpose and not introduce a new name.

gilles-peskine-arm · 2020-11-14T11:25:10Z

scripts/config.pl

@@ -218,7 +223,7 @@

 my $done;
 for my $line (@config_lines) {
-    if ($action eq "full" || $action eq "realfull" || $action eq "baremetal" ) {
+    if (grep {$action eq $_} qw(docfull full realfull baremetal)) {
        if ($line =~ /name SECTION: Module configuration options/) {


Note that including “Module configuration options” when generating the documentation fixes the bug that some configuration options are missing (#520), but introduces a minor bug: many options have lines like //#define MBEDTLS_FOO 42 /**< Description */ that were copied from another header with // prepended, and this results in a duplicated sentence in the rendered documentation since Doxygen concatenates the documentation blocks from config.h and the other header.

mpg · 2022-11-21T09:47:42Z

A couple of years later, it was not entirely clear to me what exactly this was doing, so I ran make apidoc both before and after this PR, and the main difference is that after this PR, ecp_internal.h and rsa_internal.h are included in the generated documentation. This seems desirable as those files can be used by ALT implementors.

However since 3.0 those files have moved and are now library/ecp_internal_alt.h and library/rsa_alt_helpers.h so this PR would probably need some rework in order to find them in their new location - in addition to the conflicts that have grown in the meantime.

It should also be noted that the ALT framework is going to be deprecated in favour of PSA crypto drivers in the near future.

tom-daubney-arm · 2023-05-18T17:57:06Z

As part of our review of historical PRs we have made the decision to convert older PRs that have not been updated in 3 months into drafts until they are worked on again.

gilles-peskine-arm requested review from mpg, andresag01 and yanesca June 15, 2017 16:50

gilles-peskine-arm mentioned this pull request Jun 22, 2017

ecp_internal.h is not described in documentation #972

Closed

RonEld added the mbed TLS team label Jun 22, 2017

simonbutcher previously approved these changes Oct 1, 2017

View reviewed changes

simonbutcher requested a review from Patater October 1, 2017 12:05

simonbutcher added the needs-review Every commit must be reviewed by at least two team members, label Oct 1, 2017

Patater reviewed Oct 10, 2017

View reviewed changes

gilles-peskine-arm mentioned this pull request Oct 31, 2017

Abstract fs #1029

Closed

4 tasks

RonEld mentioned this pull request Nov 1, 2017

ecp_internal.h results in undefined reference #1147

Closed

gilles-peskine-arm requested review from Patater and removed request for andresag01 and yanesca November 27, 2017 19:23

mpg requested changes Jan 3, 2018

View reviewed changes

gilles-peskine-arm added needs-work needs-backports Backports are missing or are pending review and approval. and removed needs-review Every commit must be reviewed by at least two team members, labels Apr 17, 2018

Patater reviewed May 11, 2018

View reviewed changes

simonbutcher added the component-platform Portability layer and build scripts label May 22, 2018

gilles-peskine-arm mentioned this pull request Sep 21, 2018

2.1 only: add tests/scripts/doxygen.sh and fix make apidoc #2011

Merged

gilles-peskine-arm and others added 4 commits January 11, 2019 21:54

Include functions with ALT implementations in the Doxygen output

2fefd55

Functions which had alternative implementations were omitted from the documentation generated by apidoc_full.sh, due to the way #define's were managed. This commit fixes this except for the ECC functions.

Fix ChangeLog for apidoc_full.sh fix for full docs

2191bbd

Fix the tense in the ChangeLog and refine the language.

Clarify exclusion pattern in mbedtls.doxyfile

d4b5e81

Added a comment to doxygen/mbedtls.doxyfile to explain what is and isn't excluded from doxygen documentation and why.

simonbutcher dismissed their stale review via d4b5e81 January 12, 2019 00:21

simonbutcher force-pushed the IOTSSL-1447/development branch from 4b022b3 to d4b5e81 Compare January 12, 2019 00:21

mpg self-requested a review January 25, 2019 10:53

mpg removed their request for review February 20, 2020 11:11

gilles-peskine-arm commented Nov 14, 2020

View reviewed changes

daverodgman removed the mbed TLS team label May 13, 2022

tom-cosgrove-arm added the historical-reviewing Currently reviewing (for legacy PR/issues) label Jul 1, 2022

tom-cosgrove-arm assigned mpg Dec 9, 2022

tom-daubney-arm marked this pull request as draft May 18, 2023 17:52

tom-daubney-arm added historical-reviewed Reviewed & agreed to keep legacy PR/issue and removed historical-reviewing Currently reviewing (for legacy PR/issues) labels May 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed missing functions from doxygen headers #965

Fixed missing functions from doxygen headers #965

gilles-peskine-arm commented Jun 15, 2017

RonEld commented Jun 22, 2017

gilles-peskine-arm commented Jun 22, 2017

simonbutcher commented Oct 1, 2017

Patater Oct 10, 2017

simonbutcher Jan 12, 2019

Patater Oct 10, 2017

Patater Oct 10, 2017

simonbutcher Jan 12, 2019

Patater Oct 10, 2017

mpg Oct 10, 2017

simonbutcher Jan 11, 2019

Patater Oct 10, 2017

RonEld Nov 1, 2017

simonbutcher Jan 11, 2019

Patater Oct 10, 2017

mpg Jan 3, 2018

simonbutcher Jan 11, 2019

gilles-peskine-arm commented Nov 27, 2017

Patater commented Nov 28, 2017

mpg commented Nov 30, 2017

mpg left a comment •

edited

Loading

Patater left a comment

hanno-becker commented Aug 24, 2018

gilles-peskine-arm commented Aug 24, 2018

RonEld commented Oct 25, 2018 •

edited

Loading

simonbutcher commented Jan 9, 2019

gilles-peskine-arm commented Jan 9, 2019

simonbutcher commented Jan 12, 2019

gilles-peskine-arm Nov 14, 2020

gilles-peskine-arm Nov 14, 2020

mpg commented Nov 21, 2022

tom-daubney-arm commented May 18, 2023

Fixed missing functions from doxygen headers #965

Are you sure you want to change the base?

Fixed missing functions from doxygen headers #965

Conversation

gilles-peskine-arm commented Jun 15, 2017

RonEld commented Jun 22, 2017

gilles-peskine-arm commented Jun 22, 2017

simonbutcher commented Oct 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Nov 27, 2017

Patater commented Nov 28, 2017

mpg commented Nov 30, 2017

mpg left a comment • edited Loading

Choose a reason for hiding this comment

Patater left a comment

Choose a reason for hiding this comment

hanno-becker commented Aug 24, 2018

gilles-peskine-arm commented Aug 24, 2018

RonEld commented Oct 25, 2018 • edited Loading

simonbutcher commented Jan 9, 2019

gilles-peskine-arm commented Jan 9, 2019

simonbutcher commented Jan 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpg commented Nov 21, 2022

tom-daubney-arm commented May 18, 2023

mpg left a comment •

edited

Loading

RonEld commented Oct 25, 2018 •

edited

Loading