Specifying CFLAGS for a directory on the command line

Specifying CFLAGS for a directory on the command line

[Kent Overstreet]

Hello kbuild maintainers & list,

Years ago I used to be able to specify additional CFLAGS for a specific
subdirectory on the command line, which I used for enabling gcov
profiling without editing the subdirectory makefile, like so:

  make GCOV_PROFILE_fs_bcachefs=y

But this stopped working ages ago, I believe in the 3.x era. Does anyone
know if there's a more modern way to do this?

Cheers,
Kent

Re: Specifying CFLAGS for a directory on the command line

[Nick Desaulniers]

On Fri, Jun 9, 2023 at 3:23 PM Kent Overstreet
<kent.overstreet@linux.dev> wrote:
>
> Hello kbuild maintainers & list,
>
> Years ago I used to be able to specify additional CFLAGS for a specific

Probably cause it's KCFLAGS ;)

I used this yesterday, it works.

> subdirectory on the command line, which I used for enabling gcov
> profiling without editing the subdirectory makefile, like so:
>
>   make GCOV_PROFILE_fs_bcachefs=y
>
> But this stopped working ages ago, I believe in the 3.x era. Does anyone
> know if there's a more modern way to do this?
>
> Cheers,
> Kent



-- 
Thanks,
~Nick Desaulniers

Re: Specifying CFLAGS for a directory on the command line

[Nick Desaulniers]

On Fri, Jun 9, 2023 at 4:11 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> On Fri, Jun 9, 2023 at 3:23 PM Kent Overstreet
> <kent.overstreet@linux.dev> wrote:
> >
> > Hello kbuild maintainers & list,
> >
> > Years ago I used to be able to specify additional CFLAGS for a specific
>
> Probably cause it's KCFLAGS ;)
>
> I used this yesterday, it works.
>
> > subdirectory on the command line, which I used for enabling gcov

Ah, for a specific subdir? No I've not seen that, at least from the
command line.  Such flags can be specified via Makefile
`subdir-ccflags-y`.

> > profiling without editing the subdirectory makefile, like so:
> >
> >   make GCOV_PROFILE_fs_bcachefs=y
> >
> > But this stopped working ages ago, I believe in the 3.x era. Does anyone
> > know if there's a more modern way to do this?
> >
> > Cheers,
> > Kent
>
>
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Thanks,
~Nick Desaulniers

Re: Specifying CFLAGS for a directory on the command line

[Kent Overstreet]

Adding Peter to the cc, because I just realized gcov has a maintainer :)

On Fri, Jun 09, 2023 at 04:12:56PM -0700, Nick Desaulniers wrote:
> On Fri, Jun 9, 2023 at 4:11 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
> >
> > On Fri, Jun 9, 2023 at 3:23 PM Kent Overstreet
> > <kent.overstreet@linux.dev> wrote:
> > >
> > > Hello kbuild maintainers & list,
> > >
> > > Years ago I used to be able to specify additional CFLAGS for a specific
> >
> > Probably cause it's KCFLAGS ;)
> >
> > I used this yesterday, it works.
> >
> > > subdirectory on the command line, which I used for enabling gcov
> 
> Ah, for a specific subdir? No I've not seen that, at least from the
> command line.  Such flags can be specified via Makefile
> `subdir-ccflags-y`.

Sorry, not CFLAGS, I misread my old code - it's just a make variable.

From Documentation/dev-tools/gcov.rst, you enable gcov on a specific
subdirectory by editing that directory's makefile, and adding

GCOV_PROFILE := y

or, for a specific file within that directory,

GCOV_PROFILE_main.o := y

So, if appending a file to GCOV_PROFILE works, why not a path?

This used to work - my old code would pass GCOV_PROFILE_fs_bcachefs=y on
the make command line, but doesn't anymore.

Alas I have nowhere near the make-fu to debug this, and I believe I
tried to bisect this back in the day but got nowhere... :)

Re: Specifying CFLAGS for a directory on the command line

[Peter Oberparleiter]

On 10.06.2023 01:36, Kent Overstreet wrote:
> Adding Peter to the cc, because I just realized gcov has a maintainer :)
> 
> On Fri, Jun 09, 2023 at 04:12:56PM -0700, Nick Desaulniers wrote:
>> On Fri, Jun 9, 2023 at 4:11 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
>>>
>>> On Fri, Jun 9, 2023 at 3:23 PM Kent Overstreet
>>> <kent.overstreet@linux.dev> wrote:
>>>>
>>>> Hello kbuild maintainers & list,
>>>>
>>>> Years ago I used to be able to specify additional CFLAGS for a specific
>>>
>>> Probably cause it's KCFLAGS ;)
>>>
>>> I used this yesterday, it works.
>>>
>>>> subdirectory on the command line, which I used for enabling gcov
>>
>> Ah, for a specific subdir? No I've not seen that, at least from the
>> command line.  Such flags can be specified via Makefile
>> `subdir-ccflags-y`.
> 
> Sorry, not CFLAGS, I misread my old code - it's just a make variable.
> 
> From Documentation/dev-tools/gcov.rst, you enable gcov on a specific
> subdirectory by editing that directory's makefile, and adding
> 
> GCOV_PROFILE := y
> 
> or, for a specific file within that directory,
> 
> GCOV_PROFILE_main.o := y
> 
> So, if appending a file to GCOV_PROFILE works, why not a path?
> 
> This used to work - my old code would pass GCOV_PROFILE_fs_bcachefs=y on
> the make command line, but doesn't anymore.

I'm unaware of any kbuild support for setting GCOV_PROFILE for a
specific sub-directory from the command line, only from within the
associated Makefile. I'm not sure how this could have worked in the past
with the provide sample command line.

Here's how GCOV_PROFILE evaluation works (from scripts/Makefile.lib):

ifeq ($(CONFIG_GCOV_KERNEL),y)
_c_flags += $(if $(patsubst n%,, \
$(GCOV_PROFILE_$(basetarget).o)$(GCOV_PROFILE)$(CONFIG_GCOV_PROFILE_ALL)),\
$(CFLAGS_GCOV))
endif

This bit of Makefile code determines whether to add the flags needed to
enabled gcov profiling (CFLAGS_GCOV) to the compiler flags for the
current compilation unit (_c_flags) by looking at the concatenation of
the following variables:

- GCOV_PROFILE_<target base name>.o
- GCOV_PROFILE
- CONFIG_GCOV_PROFILE_ALL

gcov flags are only added if this concatenation does not start with an
"n", and at least one of these variables is set to a non-empty value
other than "n" ("y" typically). The "starts with" part is required to
enable precedence for the more specific variable, e.g. an "n" in
GCOV_PROFILE_filename.o overwrites a "y" in GCOV_PROFILE.

As you can see, there is no reference to a GCOV_PROFILE variable that is
named after the sub-directory for which profiling should be enabled.

-- 
Peter Oberparleiter
Linux on IBM Z Development - IBM Germany R&D

Re: Specifying CFLAGS for a directory on the command line

[Kent Overstreet]

On Mon, Jun 12, 2023 at 06:18:35PM +0200, Peter Oberparleiter wrote:
> I'm unaware of any kbuild support for setting GCOV_PROFILE for a
> specific sub-directory from the command line, only from within the
> associated Makefile. I'm not sure how this could have worked in the past
> with the provide sample command line.
> 
> Here's how GCOV_PROFILE evaluation works (from scripts/Makefile.lib):
> 
> ifeq ($(CONFIG_GCOV_KERNEL),y)
> _c_flags += $(if $(patsubst n%,, \
> $(GCOV_PROFILE_$(basetarget).o)$(GCOV_PROFILE)$(CONFIG_GCOV_PROFILE_ALL)),\
> $(CFLAGS_GCOV))
> endif
> 
> This bit of Makefile code determines whether to add the flags needed to
> enabled gcov profiling (CFLAGS_GCOV) to the compiler flags for the
> current compilation unit (_c_flags) by looking at the concatenation of
> the following variables:
> 
> - GCOV_PROFILE_<target base name>.o
> - GCOV_PROFILE
> - CONFIG_GCOV_PROFILE_ALL
> 
> gcov flags are only added if this concatenation does not start with an
> "n", and at least one of these variables is set to a non-empty value
> other than "n" ("y" typically). The "starts with" part is required to
> enable precedence for the more specific variable, e.g. an "n" in
> GCOV_PROFILE_filename.o overwrites a "y" in GCOV_PROFILE.
> 
> As you can see, there is no reference to a GCOV_PROFILE variable that is
> named after the sub-directory for which profiling should be enabled.

I've been digging through the git history, and I would swear I
hallucinated the whole thing except I have the code in ktest for driving
gcov and I swear it used to work :)

Anyways - any thoughts on how we might implement this? I really need a
way to specify directories to enable gcov for _without_ monkey patching;
that's not a viable workflow in an automated setup.

It seems like if we can get a list of directory prefixes for a path
(e.g. given fs/bcachefs/btree_iter.o it would return fs, fs/bcachefs) it
should be possible to extend the code you referenced.

Re: Specifying CFLAGS for a directory on the command line

[Peter Oberparleiter]

On 13.06.2023 20:22, Kent Overstreet wrote:
> On Mon, Jun 12, 2023 at 06:18:35PM +0200, Peter Oberparleiter wrote:
>> I'm unaware of any kbuild support for setting GCOV_PROFILE for a
>> specific sub-directory from the command line, only from within the
>> associated Makefile. I'm not sure how this could have worked in the past
>> with the provide sample command line.
>>
>> Here's how GCOV_PROFILE evaluation works (from scripts/Makefile.lib):
>>
>> ifeq ($(CONFIG_GCOV_KERNEL),y)
>> _c_flags += $(if $(patsubst n%,, \
>> $(GCOV_PROFILE_$(basetarget).o)$(GCOV_PROFILE)$(CONFIG_GCOV_PROFILE_ALL)),\
>> $(CFLAGS_GCOV))
>> endif
>>
>> This bit of Makefile code determines whether to add the flags needed to
>> enabled gcov profiling (CFLAGS_GCOV) to the compiler flags for the
>> current compilation unit (_c_flags) by looking at the concatenation of
>> the following variables:
>>
>> - GCOV_PROFILE_<target base name>.o
>> - GCOV_PROFILE
>> - CONFIG_GCOV_PROFILE_ALL
>>
>> gcov flags are only added if this concatenation does not start with an
>> "n", and at least one of these variables is set to a non-empty value
>> other than "n" ("y" typically). The "starts with" part is required to
>> enable precedence for the more specific variable, e.g. an "n" in
>> GCOV_PROFILE_filename.o overwrites a "y" in GCOV_PROFILE.
>>
>> As you can see, there is no reference to a GCOV_PROFILE variable that is
>> named after the sub-directory for which profiling should be enabled.
> 
> I've been digging through the git history, and I would swear I
> hallucinated the whole thing except I have the code in ktest for driving
> gcov and I swear it used to work :)
> 
> Anyways - any thoughts on how we might implement this? I really need a
> way to specify directories to enable gcov for _without_ monkey patching;
> that's not a viable workflow in an automated setup.
> 
> It seems like if we can get a list of directory prefixes for a path
> (e.g. given fs/bcachefs/btree_iter.o it would return fs, fs/bcachefs) it
> should be possible to extend the code you referenced.

I'll likely not be able to implement this myself, but if you or anyone
else wants to go that route, here are my thoughts: $(src) should have
the relative source code path that is needed, so here's what needs to be
done:

1. Determine how to handle non-letter/digit/underscore characters in the
   variable name:

   a) GCOV_PROFILE_fs/bcachefs => supported by GNU make [1], though
      discouraged due to possible side-effects
   b) GCOV_PROFILE_fs_bcachefs => might cause overlays, e.g. a/b/c and
      a/b_c both have the same a_b_c suffix

   Personally I'd prefer option b)

2. Define a new Makefile variable that contains $(src) with required
   character replacements (scripts/Kbuild.include might be a good place)

3. Add $(GCOV_PROFILE_$(name_of_that_new_var)) to the code quoted above
   (scripts/Makefile.lib)

4. Document the use of this new Makefile variable in kernel/gcov/Kconfig
   and Documentation/dev-tools/gcov.rst

Since this new path-suffix version would be the first
GCOV_PROFILE-variable that is primarily intended to be specified on the
make command line and not added to a Makefile, it should probably take
precedence over all other versions. To achieve that it would need to be
specified first in the $(patsubst) statement.

Once implemented, one might also consider extending this new support to
other variables like KASAN_SANITIZE or KCOV_INSTRUMENT, since all of
these are implemented the same way. I don't know whether that's useful
in all cases though.

[1] https://www.gnu.org/software/make/manual/make.html#Using-Variables

-- 
Peter Oberparleiter
Linux on IBM Z Development - IBM Germany R&D

Re: Specifying CFLAGS for a directory on the command line

[Masahiro Yamada]

On Thu, Jun 15, 2023 at 6:54 PM Peter Oberparleiter
<oberpar@linux.ibm.com> wrote:
>
> On 13.06.2023 20:22, Kent Overstreet wrote:
> > On Mon, Jun 12, 2023 at 06:18:35PM +0200, Peter Oberparleiter wrote:
> >> I'm unaware of any kbuild support for setting GCOV_PROFILE for a
> >> specific sub-directory from the command line, only from within the
> >> associated Makefile. I'm not sure how this could have worked in the past
> >> with the provide sample command line.
> >>
> >> Here's how GCOV_PROFILE evaluation works (from scripts/Makefile.lib):
> >>
> >> ifeq ($(CONFIG_GCOV_KERNEL),y)
> >> _c_flags += $(if $(patsubst n%,, \
> >> $(GCOV_PROFILE_$(basetarget).o)$(GCOV_PROFILE)$(CONFIG_GCOV_PROFILE_ALL)),\
> >> $(CFLAGS_GCOV))
> >> endif
> >>
> >> This bit of Makefile code determines whether to add the flags needed to
> >> enabled gcov profiling (CFLAGS_GCOV) to the compiler flags for the
> >> current compilation unit (_c_flags) by looking at the concatenation of
> >> the following variables:
> >>
> >> - GCOV_PROFILE_<target base name>.o
> >> - GCOV_PROFILE
> >> - CONFIG_GCOV_PROFILE_ALL
> >>
> >> gcov flags are only added if this concatenation does not start with an
> >> "n", and at least one of these variables is set to a non-empty value
> >> other than "n" ("y" typically). The "starts with" part is required to
> >> enable precedence for the more specific variable, e.g. an "n" in
> >> GCOV_PROFILE_filename.o overwrites a "y" in GCOV_PROFILE.
> >>
> >> As you can see, there is no reference to a GCOV_PROFILE variable that is
> >> named after the sub-directory for which profiling should be enabled.
> >
> > I've been digging through the git history, and I would swear I
> > hallucinated the whole thing except I have the code in ktest for driving
> > gcov and I swear it used to work :)
> >
> > Anyways - any thoughts on how we might implement this? I really need a
> > way to specify directories to enable gcov for _without_ monkey patching;
> > that's not a viable workflow in an automated setup.
> >
> > It seems like if we can get a list of directory prefixes for a path
> > (e.g. given fs/bcachefs/btree_iter.o it would return fs, fs/bcachefs) it
> > should be possible to extend the code you referenced.
>
> I'll likely not be able to implement this myself, but if you or anyone
> else wants to go that route, here are my thoughts: $(src) should have
> the relative source code path that is needed, so here's what needs to be
> done:
>
> 1. Determine how to handle non-letter/digit/underscore characters in the
>    variable name:
>
>    a) GCOV_PROFILE_fs/bcachefs => supported by GNU make [1], though
>       discouraged due to possible side-effects
>    b) GCOV_PROFILE_fs_bcachefs => might cause overlays, e.g. a/b/c and
>       a/b_c both have the same a_b_c suffix
>
>    Personally I'd prefer option b)
>
> 2. Define a new Makefile variable that contains $(src) with required
>    character replacements (scripts/Kbuild.include might be a good place)
>
> 3. Add $(GCOV_PROFILE_$(name_of_that_new_var)) to the code quoted above
>    (scripts/Makefile.lib)
>
> 4. Document the use of this new Makefile variable in kernel/gcov/Kconfig
>    and Documentation/dev-tools/gcov.rst
>
> Since this new path-suffix version would be the first
> GCOV_PROFILE-variable that is primarily intended to be specified on the
> make command line and not added to a Makefile, it should probably take
> precedence over all other versions. To achieve that it would need to be
> specified first in the $(patsubst) statement.
>
> Once implemented, one might also consider extending this new support to
> other variables like KASAN_SANITIZE or KCOV_INSTRUMENT, since all of
> these are implemented the same way. I don't know whether that's useful
> in all cases though.
>
> [1] https://www.gnu.org/software/make/manual/make.html#Using-Variables
>
> --
> Peter Oberparleiter
> Linux on IBM Z Development - IBM Germany R&D
>


I do not think it is a sensible proposal.

Another idea could be something like
CONFIG_GCOV_PROFILE_PATH=fs/bcachefs
or isn't it possible to filter dynamically?
I think ftrace can change filtering dynamically.
I do not know if it is possible for GCOV because I do not use it.

But, the best would be to not implement it at all
until we know that it is a wide demand.
The upstream is not a place to cater to every feature request.



-- 
Best Regards
Masahiro Yamada

Re: Specifying CFLAGS for a directory on the command line

[Kent Overstreet]

On Thu, Jun 15, 2023 at 07:39:08PM +0900, Masahiro Yamada wrote:
> On Thu, Jun 15, 2023 at 6:54 PM Peter Oberparleiter
> <oberpar@linux.ibm.com> wrote:
> >
> > On 13.06.2023 20:22, Kent Overstreet wrote:
> > > On Mon, Jun 12, 2023 at 06:18:35PM +0200, Peter Oberparleiter wrote:
> > >> I'm unaware of any kbuild support for setting GCOV_PROFILE for a
> > >> specific sub-directory from the command line, only from within the
> > >> associated Makefile. I'm not sure how this could have worked in the past
> > >> with the provide sample command line.
> > >>
> > >> Here's how GCOV_PROFILE evaluation works (from scripts/Makefile.lib):
> > >>
> > >> ifeq ($(CONFIG_GCOV_KERNEL),y)
> > >> _c_flags += $(if $(patsubst n%,, \
> > >> $(GCOV_PROFILE_$(basetarget).o)$(GCOV_PROFILE)$(CONFIG_GCOV_PROFILE_ALL)),\
> > >> $(CFLAGS_GCOV))
> > >> endif
> > >>
> > >> This bit of Makefile code determines whether to add the flags needed to
> > >> enabled gcov profiling (CFLAGS_GCOV) to the compiler flags for the
> > >> current compilation unit (_c_flags) by looking at the concatenation of
> > >> the following variables:
> > >>
> > >> - GCOV_PROFILE_<target base name>.o
> > >> - GCOV_PROFILE
> > >> - CONFIG_GCOV_PROFILE_ALL
> > >>
> > >> gcov flags are only added if this concatenation does not start with an
> > >> "n", and at least one of these variables is set to a non-empty value
> > >> other than "n" ("y" typically). The "starts with" part is required to
> > >> enable precedence for the more specific variable, e.g. an "n" in
> > >> GCOV_PROFILE_filename.o overwrites a "y" in GCOV_PROFILE.
> > >>
> > >> As you can see, there is no reference to a GCOV_PROFILE variable that is
> > >> named after the sub-directory for which profiling should be enabled.
> > >
> > > I've been digging through the git history, and I would swear I
> > > hallucinated the whole thing except I have the code in ktest for driving
> > > gcov and I swear it used to work :)
> > >
> > > Anyways - any thoughts on how we might implement this? I really need a
> > > way to specify directories to enable gcov for _without_ monkey patching;
> > > that's not a viable workflow in an automated setup.
> > >
> > > It seems like if we can get a list of directory prefixes for a path
> > > (e.g. given fs/bcachefs/btree_iter.o it would return fs, fs/bcachefs) it
> > > should be possible to extend the code you referenced.
> >
> > I'll likely not be able to implement this myself, but if you or anyone
> > else wants to go that route, here are my thoughts: $(src) should have
> > the relative source code path that is needed, so here's what needs to be
> > done:
> >
> > 1. Determine how to handle non-letter/digit/underscore characters in the
> >    variable name:
> >
> >    a) GCOV_PROFILE_fs/bcachefs => supported by GNU make [1], though
> >       discouraged due to possible side-effects
> >    b) GCOV_PROFILE_fs_bcachefs => might cause overlays, e.g. a/b/c and
> >       a/b_c both have the same a_b_c suffix
> >
> >    Personally I'd prefer option b)
> >
> > 2. Define a new Makefile variable that contains $(src) with required
> >    character replacements (scripts/Kbuild.include might be a good place)
> >
> > 3. Add $(GCOV_PROFILE_$(name_of_that_new_var)) to the code quoted above
> >    (scripts/Makefile.lib)
> >
> > 4. Document the use of this new Makefile variable in kernel/gcov/Kconfig
> >    and Documentation/dev-tools/gcov.rst
> >
> > Since this new path-suffix version would be the first
> > GCOV_PROFILE-variable that is primarily intended to be specified on the
> > make command line and not added to a Makefile, it should probably take
> > precedence over all other versions. To achieve that it would need to be
> > specified first in the $(patsubst) statement.
> >
> > Once implemented, one might also consider extending this new support to
> > other variables like KASAN_SANITIZE or KCOV_INSTRUMENT, since all of
> > these are implemented the same way. I don't know whether that's useful
> > in all cases though.
> >
> > [1] https://www.gnu.org/software/make/manual/make.html#Using-Variables
> >
> > --
> > Peter Oberparleiter
> > Linux on IBM Z Development - IBM Germany R&D
> >
> 
> 
> I do not think it is a sensible proposal.
> 
> Another idea could be something like
> CONFIG_GCOV_PROFILE_PATH=fs/bcachefs

With that it would only be possible to enable profiling for a single
subdirectory, and that's not going to be enough; when doing code
coverage analysis of a module we may want to profile library code/other
modules it depends on.

> or isn't it possible to filter dynamically?
> I think ftrace can change filtering dynamically.
> I do not know if it is possible for GCOV because I do not use it.

gcov requires different build flags, so no, not possible.

Re: Specifying CFLAGS for a directory on the command line

[Kent Overstreet]

On Thu, Jun 15, 2023 at 11:54:23AM +0200, Peter Oberparleiter wrote:
> I'll likely not be able to implement this myself, but if you or anyone
> else wants to go that route, here are my thoughts: $(src) should have
> the relative source code path that is needed, so here's what needs to be
> done:
> 
> 1. Determine how to handle non-letter/digit/underscore characters in the
>    variable name:
> 
>    a) GCOV_PROFILE_fs/bcachefs => supported by GNU make [1], though
>       discouraged due to possible side-effects
>    b) GCOV_PROFILE_fs_bcachefs => might cause overlays, e.g. a/b/c and
>       a/b_c both have the same a_b_c suffix
> 
>    Personally I'd prefer option b)

Agreed, feels more consistent

> 2. Define a new Makefile variable that contains $(src) with required
>    character replacements (scripts/Kbuild.include might be a good place)
> 
> 3. Add $(GCOV_PROFILE_$(name_of_that_new_var)) to the code quoted above
>    (scripts/Makefile.lib)

So this is where I was getting stuck, because we really want this to
apply to subdirectories (e.g. GCOV_PROFILE_fs_xfs should really also
apply to fs/xfs/libxfs).

Do we have existing code for generating a list of path prefixes for a
given path?

Re: Specifying CFLAGS for a directory on the command line

[Peter Oberparleiter]

On 15.06.2023 12:39, Masahiro Yamada wrote:
> On Thu, Jun 15, 2023 at 6:54 PM Peter Oberparleiter
> <oberpar@linux.ibm.com> wrote:
>>
>> On 13.06.2023 20:22, Kent Overstreet wrote:
>>> On Mon, Jun 12, 2023 at 06:18:35PM +0200, Peter Oberparleiter wrote:
>>>> I'm unaware of any kbuild support for setting GCOV_PROFILE for a
>>>> specific sub-directory from the command line, only from within the
>>>> associated Makefile. I'm not sure how this could have worked in the past
>>>> with the provide sample command line.
>>>>
>>>> Here's how GCOV_PROFILE evaluation works (from scripts/Makefile.lib):
>>>>
>>>> ifeq ($(CONFIG_GCOV_KERNEL),y)
>>>> _c_flags += $(if $(patsubst n%,, \
>>>> $(GCOV_PROFILE_$(basetarget).o)$(GCOV_PROFILE)$(CONFIG_GCOV_PROFILE_ALL)),\
>>>> $(CFLAGS_GCOV))
>>>> endif
>>>>
>>>> This bit of Makefile code determines whether to add the flags needed to
>>>> enabled gcov profiling (CFLAGS_GCOV) to the compiler flags for the
>>>> current compilation unit (_c_flags) by looking at the concatenation of
>>>> the following variables:
>>>>
>>>> - GCOV_PROFILE_<target base name>.o
>>>> - GCOV_PROFILE
>>>> - CONFIG_GCOV_PROFILE_ALL
>>>>
>>>> gcov flags are only added if this concatenation does not start with an
>>>> "n", and at least one of these variables is set to a non-empty value
>>>> other than "n" ("y" typically). The "starts with" part is required to
>>>> enable precedence for the more specific variable, e.g. an "n" in
>>>> GCOV_PROFILE_filename.o overwrites a "y" in GCOV_PROFILE.
>>>>
>>>> As you can see, there is no reference to a GCOV_PROFILE variable that is
>>>> named after the sub-directory for which profiling should be enabled.
>>>
>>> I've been digging through the git history, and I would swear I
>>> hallucinated the whole thing except I have the code in ktest for driving
>>> gcov and I swear it used to work :)
>>>
>>> Anyways - any thoughts on how we might implement this? I really need a
>>> way to specify directories to enable gcov for _without_ monkey patching;
>>> that's not a viable workflow in an automated setup.
>>>
>>> It seems like if we can get a list of directory prefixes for a path
>>> (e.g. given fs/bcachefs/btree_iter.o it would return fs, fs/bcachefs) it
>>> should be possible to extend the code you referenced.
>>
>> I'll likely not be able to implement this myself, but if you or anyone
>> else wants to go that route, here are my thoughts: $(src) should have
>> the relative source code path that is needed, so here's what needs to be
>> done:
>>
>> 1. Determine how to handle non-letter/digit/underscore characters in the
>>    variable name:
>>
>>    a) GCOV_PROFILE_fs/bcachefs => supported by GNU make [1], though
>>       discouraged due to possible side-effects
>>    b) GCOV_PROFILE_fs_bcachefs => might cause overlays, e.g. a/b/c and
>>       a/b_c both have the same a_b_c suffix
>>
>>    Personally I'd prefer option b)
>>
>> 2. Define a new Makefile variable that contains $(src) with required
>>    character replacements (scripts/Kbuild.include might be a good place)
>>
>> 3. Add $(GCOV_PROFILE_$(name_of_that_new_var)) to the code quoted above
>>    (scripts/Makefile.lib)
>>
>> 4. Document the use of this new Makefile variable in kernel/gcov/Kconfig
>>    and Documentation/dev-tools/gcov.rst
>>
[...]

> I do not think it is a sensible proposal.

Does your objection apply to the proposed implementation, or the feature
request itself? If the former, what aspect of the approach do you
consider problematic?

> Another idea could be something like
> CONFIG_GCOV_PROFILE_PATH=fs/bcachefs
> or isn't it possible to filter dynamically?

Using CONFIG symbols would be another way to specify the input list of
directories that should be instrumented for GCOV profiling. The output
would still need to be a different c_flags value for source files in
these directories though. I would assume that getting this information
from CONFIG values into c_flags would be a more intrusive change
compared to the route via a Makefile variable.

> I think ftrace can change filtering dynamically.
> I do not know if it is possible for GCOV because I do not use it.

GCOV instrumentation is a per-source file build-time decision. There's
no way to dynamically turn it on or off during run-time.

> But, the best would be to not implement it at all
> until we know that it is a wide demand.
> The upstream is not a place to cater to every feature request.

I don't feel very strongly about this specific feature - it seemed to me
that it could be implemented in a non-intrusive way and it provides
value to at least one developer.


-- 
Peter Oberparleiter
Linux on IBM Z Development - IBM Germany R&D

Re: Specifying CFLAGS for a directory on the command line

[Kent Overstreet]

On Fri, Jun 16, 2023 at 06:45:46PM +0200, Peter Oberparleiter wrote:
> I don't feel very strongly about this specific feature - it seemed to me
> that it could be implemented in a non-intrusive way and it provides
> value to at least one developer.

And it'll get us better tooling integration.

I'm the author of ktest [1], which does interactive and automated kernel
testing. With this feature I'll be able to generate gcov/lcov output
with a single argument to build-test-kernel, and hopefully with a bit of
extra work get code coverage analysis as part of CI runs.

[1] https://evilpiepirate.org/git/ktest.git

Re: Specifying CFLAGS for a directory on the command line

[Peter Oberparleiter]

On 16.06.2023 08:37, Kent Overstreet wrote:
> On Thu, Jun 15, 2023 at 11:54:23AM +0200, Peter Oberparleiter wrote:
>> I'll likely not be able to implement this myself, but if you or anyone
>> else wants to go that route, here are my thoughts: $(src) should have
>> the relative source code path that is needed, so here's what needs to be
>> done:
>>
>> 1. Determine how to handle non-letter/digit/underscore characters in the
>>    variable name:
>>
>>    a) GCOV_PROFILE_fs/bcachefs => supported by GNU make [1], though
>>       discouraged due to possible side-effects
>>    b) GCOV_PROFILE_fs_bcachefs => might cause overlays, e.g. a/b/c and
>>       a/b_c both have the same a_b_c suffix
>>
>>    Personally I'd prefer option b)
> 
> Agreed, feels more consistent
> 
>> 2. Define a new Makefile variable that contains $(src) with required
>>    character replacements (scripts/Kbuild.include might be a good place)
>>
>> 3. Add $(GCOV_PROFILE_$(name_of_that_new_var)) to the code quoted above
>>    (scripts/Makefile.lib)
> 
> So this is where I was getting stuck, because we really want this to
> apply to subdirectories (e.g. GCOV_PROFILE_fs_xfs should really also
> apply to fs/xfs/libxfs).
> 
> Do we have existing code for generating a list of path prefixes for a
> given path?

Not that I know of. Here's how it could be made to work using Makefile
magic alone (as a pure programming exercise :)

This will expand a directory to a list of all parent directories:

# expand_parents(a/b/c) = a/b/c a/b a
expand_parents2 = $(if $(subst .,,$(1)),$(call expand_parents,$(1)),)
expand_parents  = $(1) $(call expand_parents2,$(patsubst %/,%,$(dir $(1))))

This list could then be turned into variable suffixes:

# flatten_dirs(a/b/c) = a_b_c a_b a
flatten_dirs = $(subst /,_,$(call expand_parents,$(1)))

And finally the resulting list of suffixed variables could be evaluated:

# eval_vars(X_,a/b/c) = $(X_a_b_c) $(X_a_b) $(X_a)
eval_vars = $(foreach var,$(call flatten_dirs,$(2)),$($(1)$(var)))

So a call like this

$(call eval_vars,GCOV_PROFILE_,a/b/c)

would evaluate to the concatenation of the contents of the following
variables:

GCOV_PROFILE_a_b_c
GCOV_PROFILE_a_b
GCOV_PROFILE_a

The first non-empty variable would then determine whether profiling
should be enabled for the associated source file or not. This would even
implement the correct order of precedence (specific to generic)

Not sure if this amount of magic is suitable for kbuild though. An
alternative, less complex approach would be to move this decision logic
to a helper script.


-- 
Peter Oberparleiter
Linux on IBM Z Development - IBM Germany R&D

Re: Specifying CFLAGS for a directory on the command line

[Kent Overstreet]

On Fri, Jun 16, 2023 at 08:10:44PM +0200, Peter Oberparleiter wrote:
> Not that I know of. Here's how it could be made to work using Makefile
> magic alone (as a pure programming exercise :)
> 
> This will expand a directory to a list of all parent directories:
> 
> # expand_parents(a/b/c) = a/b/c a/b a
> expand_parents2 = $(if $(subst .,,$(1)),$(call expand_parents,$(1)),)
> expand_parents  = $(1) $(call expand_parents2,$(patsubst %/,%,$(dir $(1))))
> 
> This list could then be turned into variable suffixes:
> 
> # flatten_dirs(a/b/c) = a_b_c a_b a
> flatten_dirs = $(subst /,_,$(call expand_parents,$(1)))
> 
> And finally the resulting list of suffixed variables could be evaluated:
> 
> # eval_vars(X_,a/b/c) = $(X_a_b_c) $(X_a_b) $(X_a)
> eval_vars = $(foreach var,$(call flatten_dirs,$(2)),$($(1)$(var)))
> 
> So a call like this
> 
> $(call eval_vars,GCOV_PROFILE_,a/b/c)

I just hooked it up and it works perfectly - I owe you a beer :)

> would evaluate to the concatenation of the contents of the following
> variables:
> 
> GCOV_PROFILE_a_b_c
> GCOV_PROFILE_a_b
> GCOV_PROFILE_a
> 
> The first non-empty variable would then determine whether profiling
> should be enabled for the associated source file or not. This would even
> implement the correct order of precedence (specific to generic)
> 
> Not sure if this amount of magic is suitable for kbuild though. An
> alternative, less complex approach would be to move this decision logic
> to a helper script.

Now that I've spent a couple days starting to wrap my head more around
make, it doesn't look terribly magic to me. I'd hate to have to spawn
off a subshell for this.

Re: Specifying CFLAGS for a directory on the command line

[Peter Oberparleiter]

On 17.06.2023 02:28, Kent Overstreet wrote:
> On Fri, Jun 16, 2023 at 08:10:44PM +0200, Peter Oberparleiter wrote:
>> Not that I know of. Here's how it could be made to work using Makefile
>> magic alone (as a pure programming exercise :)
>>
>> This will expand a directory to a list of all parent directories:
>>
>> # expand_parents(a/b/c) = a/b/c a/b a
>> expand_parents2 = $(if $(subst .,,$(1)),$(call expand_parents,$(1)),)
>> expand_parents  = $(1) $(call expand_parents2,$(patsubst %/,%,$(dir $(1))))
>>
>> This list could then be turned into variable suffixes:
>>
>> # flatten_dirs(a/b/c) = a_b_c a_b a
>> flatten_dirs = $(subst /,_,$(call expand_parents,$(1)))
>>
>> And finally the resulting list of suffixed variables could be evaluated:
>>
>> # eval_vars(X_,a/b/c) = $(X_a_b_c) $(X_a_b) $(X_a)
>> eval_vars = $(foreach var,$(call flatten_dirs,$(2)),$($(1)$(var)))
>>
>> So a call like this
>>
>> $(call eval_vars,GCOV_PROFILE_,a/b/c)
> 
> I just hooked it up and it works perfectly - I owe you a beer :)

Glad to be of help!

>> would evaluate to the concatenation of the contents of the following
>> variables:
>>
>> GCOV_PROFILE_a_b_c
>> GCOV_PROFILE_a_b
>> GCOV_PROFILE_a
>>
>> The first non-empty variable would then determine whether profiling
>> should be enabled for the associated source file or not. This would even
>> implement the correct order of precedence (specific to generic)
>>
>> Not sure if this amount of magic is suitable for kbuild though. An
>> alternative, less complex approach would be to move this decision logic
>> to a helper script.
> 
> Now that I've spent a couple days starting to wrap my head more around
> make, it doesn't look terribly magic to me. I'd hate to have to spawn
> off a subshell for this.

Leaving the method of how this input data is processed inside kbuild
aside for a moment, I'm wondering if specifying the list of directories
via a CONFIG symbol instead of make parameters would work equally well.

Initially I thought this would be too complex to process using Makefile
functions alone, but with the logic shown above this could be relatively
simple to achieve. Also having given this some more thought, a CONFIG
symbol indeed seems like a better fit considering aspects such as
reproducibility of builds and config symbol documentation.

The CONFIG symbols could look something like:

- CONFIG_GCOV_PROFILE_INCLUDE: list of directories to include in
  profiling
- CONFIG_GCOV_PROFILE_EXCLUDE: list of directories to exclude from
  profiling

where the precedence would be: exclude list > include list >
GCOV_PROFILE_* specified in Makefiles > CONFIG_GCOV_PROFILE_ALL

Sub-directory handling could work similar to how you described it for
the make parameter, i.e. an include/exclude statement for a parent
directory would also apply to sub-directories.

What this approach work for your use case? Note that I'm not asking you
to implement this - I just want to get a better picture of how a generic
solution could look like.

-- 
Peter Oberparleiter
Linux on IBM Z Development - IBM Germany R&D

Re: Specifying CFLAGS for a directory on the command line

[Kent Overstreet]

On Wed, Jun 21, 2023 at 10:54:10AM +0200, Peter Oberparleiter wrote:
> Leaving the method of how this input data is processed inside kbuild
> aside for a moment, I'm wondering if specifying the list of directories
> via a CONFIG symbol instead of make parameters would work equally well.
> 
> Initially I thought this would be too complex to process using Makefile
> functions alone, but with the logic shown above this could be relatively
> simple to achieve. Also having given this some more thought, a CONFIG
> symbol indeed seems like a better fit considering aspects such as
> reproducibility of builds and config symbol documentation.
> 
> The CONFIG symbols could look something like:
> 
> - CONFIG_GCOV_PROFILE_INCLUDE: list of directories to include in
>   profiling
> - CONFIG_GCOV_PROFILE_EXCLUDE: list of directories to exclude from
>   profiling
> 
> where the precedence would be: exclude list > include list >
> GCOV_PROFILE_* specified in Makefiles > CONFIG_GCOV_PROFILE_ALL
> 
> Sub-directory handling could work similar to how you described it for
> the make parameter, i.e. an include/exclude statement for a parent
> directory would also apply to sub-directories.
> 
> What this approach work for your use case? Note that I'm not asking you
> to implement this - I just want to get a better picture of how a generic
> solution could look like.

Yeah, that would work fine. I'm assuming the config option list would
just be a comma separated string?

btw, here's the results - fully automated code coverage analysis
integrated into the CI:
https://evilpiepirate.org/~testdashboard/ci?branch=bcachefs&commit=eac40840098bfbb5fa1711f6bbce71b27bbccb89
https://evilpiepirate.org/~testdashboard/c/eac40840098bfbb5fa1711f6bbce71b27bbccb89/lcov/

And the result of the makefile work is that adding a gcov variant of an
existing test is as easy as:
https://evilpiepirate.org/git/ktest.git/tree/tests/bcachefs/gcov-xfstests.ktest