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

Documentation not indexed #353

Open
Fomys opened this issue Nov 25, 2024 · 7 comments
Open

Documentation not indexed #353

Fomys opened this issue Nov 25, 2024 · 7 comments

Comments

@Fomys
Copy link
Contributor

Fomys commented Nov 25, 2024

Hi, I just found few documentation that are not indexed by elixir:

https://elixir.bootlin.com/linux/v6.12.1/source/mm/util.c#L55 - Because of "noinline"?

https://elixir.bootlin.com/linux/v6.12.1/source/mm/util.c#L817 - Because the name in kdoc is wrong?

Thanks
Louis Chauvet

@tleb
Copy link
Member

tleb commented Nov 25, 2024

Can you explicit what is missing? I guess you are talking about arguments not being tagged as references but I am not sure.

For the first link it would be char and s.
For the second link it would be n and size.

Thanks!


edit: no, it seems you are talking about doc comments. That is related to find-file-doc-comments.pl, which we don't know much about. We need to look into it.

@cxw42
Copy link
Contributor

cxw42 commented Nov 25, 2024

For the second one, I think it is because of the name mismatch. The code looks for a matching name as a shortcut to avoid needing a full C parser.

qr{^$comment_leader\Q$definition_name\E(?:\h|\(|:|$)};

@cxw42
Copy link
Contributor

cxw42 commented Nov 25, 2024

For the first one, it might be that the ctags invocation needs to be updated.

my @ctags = qx{ ctags -x --c-kinds=+p-m --language-force=C "$filename" |

@tleb
Copy link
Member

tleb commented Nov 26, 2024

For the first one, it might be that the ctags invocation needs to be updated.

Indeed the ctags inside the Docker container doesn't give an entry for strdup. Inside the Docker container:

$ git -C elixir rev-parse --short HEAD
5fe2a8e

$ docker build -t elixir -f ./elixir/docker/Dockerfile ./elixir
...

$ docker run --rm -it --entrypoint /bin/bash elixir

# wget -O util.c "https://elixir.bootlin.com/linux/v6.12.1/source/mm/util.c?raw"

# ctags -x --c-kinds=+p-m --language-force=C util.c | grep ^strdup

# ctags --version
Universal Ctags 5.9.0, Copyright (C) 2015 Universal Ctags Team
Universal Ctags is derived from Exuberant Ctags.
Exuberant Ctags 5.8, Copyright (C) 1996-2009 Darren Hiebert
  Compiled: Sep  3 2021, 18:12:18
  URL: https://ctags.io/
  Optional compiled features: +wildcards, +regex, +gnulib_regex, +iconv, +option-directory, +xpath, +json, +interactive, +sandbox, +yaml, +packcc, +optscript

Compared to a recent ctags (the one used by the prod server):

$ wget -O util.c "https://elixir.bootlin.com/linux/v6.12.1/source/mm/util.c?raw"

$ $ctags -x --c-kinds=+p-m --language-force=C util.c  | grep -w ^kstrdup
kstrdup          function     55 util.c           char *kstrdup(const char *s, gfp_t gfp)

$ $ctags --version
Universal Ctags 6.1.0, Copyright (C) 2015-2023 Universal Ctags Team
Universal Ctags is derived from Exuberant Ctags.
Exuberant Ctags 5.8, Copyright (C) 1996-2009 Darren Hiebert
  Compiled: Nov  8 2024, 17:48:48
  URL: https://ctags.io/
  Output version: 0.0
  Optional compiled features: +wildcards, +regex, +gnulib_regex, +iconv, +option-directory, +yaml, +packcc, +optscript, +pcre2

This is an issue with the universal-ctags packaged by Debian Bookworm:

$ docker run --rm -it --entrypoint /bin/bash debian:bookworm
# apt-get update && apt-get upgrade -y && apt-get install -y wget universal-ctags
# wget -O util.c "https://elixir.bootlin.com/linux/v6.12.1/source/mm/util.c?raw"
# ctags -x --c-kinds=+p-m --language-force=C util.c | grep ^strdup

It doesn't work with debian:testing nor debian:unstable. The debian package index agrees (all have the same version).

So prod server has a recent ctags that works fine, but the initial indexing has been done inside the Docker container. That means blobs that didn't change are wrong in the latest version. To address this, we should:

  1. fix the docker image to have a recent enough universal-ctags version and,
  2. do a full reindex.

For (1) we could either switch distro, sideload universal-ctags or compile it manually.

@tleb
Copy link
Member

tleb commented Nov 26, 2024

For the second one, I think it is because of the name mismatch. The code looks for a matching name as a shortcut to avoid needing a full C parser.

For this one, I am wondering if we could ignore finding a declaration/prototype below that matches the name. If we take declarations from linux/mm/util.c, there are quite a few that follow the $a_noprof with $a being declared as a wrapper macro in header files, adding some safety/profiling code surrounding the function call.

I cannot think of places where not checking the declaration below would be an issue.

@cxw42
Copy link
Contributor

cxw42 commented Nov 28, 2024

@tleb Thanks for the research on the first one!

Re. the second one, I don't recall why I added that check. I think it might have been to avoid indexing doc comments belonging to things other than functions. E.g., a doc comment for a macro, then the macro, then a function --- we don't want the doc comment associated with the function.

@tleb
Copy link
Member

tleb commented Nov 28, 2024

I am wondering if we could change the approach completely:

  • Find lines like /**,
  • Keep matches where the next line is * $ident - ...,
  • Keep matches were we have a definition for $ident in the database.

Benefits:

  • Script doesn't have to call ctags,
  • We take into account doccomments which aren't located next to the definitions (ie the current issue),
  • Simpler implementation.

Downfalls:

  • False positives? Those should be mitigated by the database definition check.
  • False negatives? Is the above too restrictive for doc comments? It might be specific to C kernel code standards for example, and different in other projects.

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

No branches or pull requests

3 participants