Re: [PATCH] kbuild: llvmlinux: Set appropriate compiler-flag for CONFIG_CC_OPTIMIZE_FOR_SIZE
On 09/14/15 22:07, Sedat Dilek wrote: On Tue, Sep 15, 2015 at 5:40 AM, Behan Webster wrote: I haven't upstreamed this patch yet because we are still characterizing its effects on other architectures. Then embed this information into the changelog of *your* patch. It's in our tree/build system, as a work in progress. Which is why I haven't upstreamed it yet. So what is *your* solution in case of CONFIG_CC_OPTIMIZE_FOR_SIZE=y (OptLevel '-Os' is wrong, '-Oz' is correct)? I don't have one yet, which is precisely why I haven't upstreamed it yet. Sedat, why don't you try working with us instead of making demands and accusing people? Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kbuild: llvmlinux: Set appropriate compiler-flag for CONFIG_CC_OPTIMIZE_FOR_SIZE
Other than you changing the commit comment, this *is* my patch. I haven't upstreamed this patch yet because we are still characterizing its effects on other architectures. Clang certainly doesn't require -Oz to be used to be able to boot for arm nor arm64. So the commit message isn't correct either. NAK Behan On 09/14/15 21:19, Sedat Dilek wrote: Based on a patch of Behan Webster (see [1]). CLANG (here: v3.7) requires '-Oz' as OptLevel to be set. A Linux v4.3-rc1 kernel built fine with CONFIG_CC_OPTIMIZE_FOR_SIZE=y and boots on bare metal. This is a Ubuntu/precise AMD64 system. Tested against Linux v4.3-rc1 and a refreshed llvmlinux patchset. [1] http://git.linuxfoundation.org/?p=llvmlinux.git;a=blob;f=arch/all/patches/smaller.patch CC: Behan Webster --- Makefile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 4249441e535d..a57fb6b39ed7 100644 --- a/Makefile +++ b/Makefile @@ -622,7 +622,8 @@ KBUILD_CFLAGS += $(call cc-option,-fno-delete-null-pointer-checks,) endif ifdef CONFIG_CC_OPTIMIZE_FOR_SIZE -KBUILD_CFLAGS += -Os $(call cc-disable-warning,maybe-uninitialized,) +KBUILD_CFLAGS += $(call cc-option,-Oz,-Os) +KBUILD_CFLAGS += $(call cc-disable-warning,maybe-uninitialized,) else KBUILD_CFLAGS += -O2 endif -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] Makefile: Fix detection of clang when cross-compiling
On 08/19/15 08:41, Michal Marek wrote:
On Mon, Jul 13, 2015 at 08:59:33PM +1000, Anton Blanchard wrote:
Hi,
> When the host's C compiler is clang, and when attempting to
> cross-compile Linux e.g. to MIPS with mipsel-linux-gcc, the
> Makefile would incorrectly detect the use of clang, which
> resulted in clang-specific flags being passed to
> mipsel-linux-gcc.
>
> This can be verified under Debian by installing the "clang"
> package, and then using it as the default compiler with:
> sudo update-alternatives --config cc
>
> This patch moves the detection of clang after the $(CC)
> variable is initialized to the name of the cross-compiler, so
> that the check applies
> to the cross-compiler and not the host's C compiler.
>
> v2: Move the detection of clang after the inclusion of the
> arch/*/Makefile (as they might set $(CROSS_COMPILE))
>
> Signed-off-by: Paul Cercueil mailto:p...@crapouillou.net>>
Applied to kbuild.git#kbuild. I will push it after v4.1-rc1
becomes available, though.
Drat. I wish I saw this earlier.
This breaks patches which check for the value of COMPILER in
arch/*/Makefile. This detection must be performed before the
inclusion of the arch Makefile.
Can I move this to after the initialization of CC but before the
include?
I'm not sure that being able to define the default compiler per arch
is necessary. But I know I need to be able to add arch specific flags
for clang.
I can confirm the patch breaks ppc64le clang builds.
Can you try the (untested) patch below?
From 49bb3e66a9a7fc3685fb070bdfd31f2c3d78cc87 Mon Sep 17 00:00:00 2001
From: Michal Marek
Date: Wed, 19 Aug 2015 17:36:41 +0200
Subject: [PATCH] kbuild: Fix clang detection
We cannot detect clang before including the arch Makefile, because that
can set the default cross compiler. We also cannot detect clang after
including the arch Makefile, because powerpc wants to know about clang.
Solve this by using an deferred variable. This costs us a few shell
invocations, but this is only a constant number.
Reported-by: Behan Webster
Reported-by: Anton Blanchard
Signed-off-by: Michal Marek
---
Makefile | 9 +
arch/powerpc/Makefile | 8
scripts/Kbuild.include | 4
scripts/Makefile.extrawarn | 2 +-
4 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/Makefile b/Makefile
index 13270c0..5ccbb58 100644
--- a/Makefile
+++ b/Makefile
@@ -661,14 +661,7 @@ endif
endif
KBUILD_CFLAGS += $(stackp-flag)
-ifeq ($(shell $(CC) -v 2>&1 | grep -c "clang version"), 1)
-COMPILER := clang
-else
-COMPILER := gcc
-endif
-export COMPILER
-
-ifeq ($(COMPILER),clang)
+ifeq ($(cc-name),clang)
KBUILD_CPPFLAGS += $(call cc-option,-Qunused-arguments,)
KBUILD_CPPFLAGS += $(call cc-option,-Wno-unknown-warning-option,)
KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable)
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index 05f464e..dfe88896 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -67,7 +67,7 @@ UTS_MACHINE := $(OLDARCH)
ifeq ($(CONFIG_CPU_LITTLE_ENDIAN),y)
override CC += -mlittle-endian
-ifneq ($(COMPILER),clang)
+ifneq ($(cc-name),clang)
override CC += -mno-strict-align
endif
override AS += -mlittle-endian
@@ -333,7 +333,7 @@ TOUT:= .tmp_gas_check
# - Require gcc 4.0 or above on 64-bit
# - gcc-4.2.0 has issues compiling modules on 64-bit
checkbin:
- @if test "${COMPILER}" != "clang" \
+ @if test "$(cc-name)" != "clang" \
&& test "$(cc-version)" = "0304" ; then \
if ! /bin/echo mftb 5 | $(AS) -v -mppc -many -o $(TOUT) >/dev/null
2>&1 ; then \
echo -n '*** ${VERSION}.${PATCHLEVEL} kernels no longer
build '; \
@@ -342,14 +342,14 @@ checkbin:
false; \
fi ; \
fi
- @if test "${COMPILER}" != "clang" \
+ @if test "$(cc-name)" != "clang" \
&& test "$(cc-version)" -lt "0400" \
&& test "x${CONFIG_PPC64}" = "xy" ; then \
echo -n "Sorry, GCC v4.0 or above is required to build " ; \
echo "the 64-bit powerpc kernel." ; \
false ; \
fi
- @if test "${COMPILER}" != "clang" \
+ @if test "$(cc-name)" != "clang" \
&& test "$(cc-fullversion)" = "040200" \
&& test "x${CONFIG_MODULES}${CONFIG_PPC64}" = "xyy" ; then \
echo -n '***
Re: [PATCH v2] Makefile: Fix detection of clang when cross-compiling
Resent since gmail HTML-ified my previous email... On Wed, Apr 22, 2015 at 7:33 AM, Michal Marek <mailto:mma...@suse.cz>> wrote: On Fri, Apr 17, 2015 at 11:35:04PM +0200, Paul Cercueil wrote: > When the host's C compiler is clang, and when attempting to > cross-compile Linux e.g. to MIPS with mipsel-linux-gcc, the Makefile > would incorrectly detect the use of clang, which resulted in > clang-specific flags being passed to mipsel-linux-gcc. > > This can be verified under Debian by installing the "clang" package, > and then using it as the default compiler with: > sudo update-alternatives --config cc > > This patch moves the detection of clang after the $(CC) variable is > initialized to the name of the cross-compiler, so that the check applies > to the cross-compiler and not the host's C compiler. > > v2: Move the detection of clang after the inclusion of the > arch/*/Makefile (as they might set $(CROSS_COMPILE)) > > Signed-off-by: Paul Cercueil mailto:p...@crapouillou.net>> Applied to kbuild.git#kbuild. I will push it after v4.1-rc1 becomes available, though. Drat. I wish I saw this earlier. This breaks patches which check for the value of COMPILER in arch/*/Makefile. This detection must be performed before the inclusion of the arch Makefile. Can I move this to after the initialization of CC but before the include? I'm not sure that being able to define the default compiler per arch is necessary. But I know I need to be able to add arch specific flags for clang. Behan -- Behan Webster beh...@converseincode.com <mailto:beh...@converseincode.com> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net, ethernet, LLVMLinux: Add missing MODULE_DEVICE_TABLE()
On 01/28/15 22:42, David Miller wrote: > From: Behan Webster > Date: Wed, 28 Jan 2015 17:36:14 -0800 > >> Missing MODULE_DEVICE_TABLE for pci ids from benet driver found by clang. >> >> Signed-off-by: Behan Webster >> Suggested-by: Arnd Bergmann > Why are you removing the device table? It is defined more than once; removing the duplicate (as Arnd indicated). My commit message was just completely wrong. Brain fart. Sorry. > Second of all, your Subject needs to be adjusted, using "net" and > "LLVMLinux" in your subsystem prefix is not appropriate. Simply > "be2net: ", the name of this driver, is sufficient. Will fix. I've been in the habit of labelling the patches which go through the LLVMLinux project like this so they are trivially identifiable in the subject on lkml for reviewers and in the git log as being patches which exist because of clang. If it's annoying I certainly don't need to do it. Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net, ethernet, LLVMLinux: Add missing MODULE_DEVICE_TABLE()
On 01/29/15 01:10, Arnd Bergmann wrote: > On Wednesday 28 January 2015 22:42:28 David Miller wrote: >> From: Behan Webster >> Date: Wed, 28 Jan 2015 17:36:14 -0800 >> >>> Missing MODULE_DEVICE_TABLE for pci ids from benet driver found by clang. >>> >>> Signed-off-by: Behan Webster >>> Suggested-by: Arnd Bergmann >> Why are you removing the device table? > Behan took a patch that I did earlier and split it up to add descriptions. > The patch is correct, but he either misunderstood or misexpressed the > intention. I was tired and rushed this submission in my preparation for FOSDEM. Apologies to all. I neglected to write the commit log when I first split the patch, and didn't look hard enough this time. > This driver has two identical lines that both say > > MODULE_DEVICE_TABLE(pci, be_dev_ids); This is indeed the case. > I don't remember the exact symptom, but llvm/clang trips over this, while gcc > silently ignores the second one. It claims that it is defined more than once. Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] net, ethernet, LLVMLinux: Add missing MODULE_DEVICE_TABLE()
Missing MODULE_DEVICE_TABLE for pci ids from benet driver found by clang.
Signed-off-by: Behan Webster
Suggested-by: Arnd Bergmann
Cc: Arnd Bergmann
---
drivers/net/ethernet/emulex/benet/be_main.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/net/ethernet/emulex/benet/be_main.c
b/drivers/net/ethernet/emulex/benet/be_main.c
index d48806b..709400a 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -26,7 +26,6 @@
#include
MODULE_VERSION(DRV_VER);
-MODULE_DEVICE_TABLE(pci, be_dev_ids);
MODULE_DESCRIPTION(DRV_DESC " " DRV_VER);
MODULE_AUTHOR("Emulex Corporation");
MODULE_LICENSE("GPL");
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] scsi, be2iscsi, LLVMLinux: Add missing MODULE_DEVICE_TABLE()
Missing MODULE_DEVICE_TABLE for pci ids from be2iscsi driver found by clang.
Signed-off-by: Behan Webster
Reviewed-by: Mark Charlebois
Suggested-by: Arnd Bergmann
Cc: Arnd Bergmann
---
drivers/scsi/be2iscsi/be_main.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index f319340..96241b2 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -48,7 +48,6 @@ static unsigned int be_iopoll_budget = 10;
static unsigned int be_max_phys_size = 64;
static unsigned int enable_msix = 1;
-MODULE_DEVICE_TABLE(pci, beiscsi_pci_id_table);
MODULE_DESCRIPTION(DRV_DESC " " BUILD_STR);
MODULE_VERSION(BUILD_STR);
MODULE_AUTHOR("Emulex Corporation");
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] bcm: address clang inline asm incompatibility
On 01/28/15 12:11, Ard Biesheuvel wrote:
> On 28 January 2015 at 19:38, Ard Biesheuvel wrote:
>> On 28 January 2015 at 19:27, Alex Elder wrote:
>>> On 01/28/2015 01:17 PM, Ard Biesheuvel wrote:
>>>> On 28 January 2015 at 17:20, Ard Biesheuvel
>>>> wrote:
>>>>> On 28 January 2015 at 17:08, Alex Elder wrote:
>>>>>> On 01/28/2015 10:17 AM, Ard Biesheuvel wrote:
>>>>>>> On 28 January 2015 at 14:11, Alex Elder wrote:
>>>>>>>> On 01/28/2015 05:15 AM, Ard Biesheuvel wrote:
>>>>>>>>> On 28 January 2015 at 05:18, Behan Webster
>>>>>>>>> wrote:
>>>>>>>>>> From: Alex Elder
>>>>>>>>>>
>>>>>>>>>> My GCC-based build environment likes to call register r12 by the
>>>>>>>>>> name "ip" in inline asm. Behan Webster informed me that his Clang-
>>>>>>>>>> based build environment likes "r12" instead.
>>>>>>>>>>
>>>>>>>>>> Try to make them both happy.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Alex Elder
>>>>>>>>>> Signed-off-by: Behan Webster
>>>>>>>>>> ---
>>>>>>>>>> arch/arm/mach-bcm/bcm_kona_smc.c | 9 +++--
>>>>>>>>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/arch/arm/mach-bcm/bcm_kona_smc.c
>>>>>>>>>> b/arch/arm/mach-bcm/bcm_kona_smc.c
>>>>>>>>>> index a55a7ec..3937bd5 100644
>>>>>>>>>> --- a/arch/arm/mach-bcm/bcm_kona_smc.c
>>>>>>>>>> +++ b/arch/arm/mach-bcm/bcm_kona_smc.c
>>>>>>>>>> @@ -106,9 +106,14 @@ int __init bcm_kona_smc_init(void)
>>>>>>>>>> * request result appropriately. This result value is found in r0
>>>>>>>>>> * when the "smc" request completes.
>>>>>>>>>> */
>>>>>>>>>> +#ifdef __clang__
>>>>>>>>>> +#define R12"r12"
>>>>>>>>>> +#else /* !__clang__ */
>>>>>>>>>> +#define R12"ip"/* gcc calls r12 "ip" */
>>>>>>>>>> +#endif /* !__clang__ */
>>>>>>>>> Why not just use r12 for both?
>>>>>>>> Yes, that would have been an obvious fix. But the
>>>>>>>> assembler (in the GCC environment) doesn't accept that.
>>>>>>>>
>>>>>>> Mine has no problems with it at all
>>>>>>>
>>>>>>> $ echo 'mov r12, #0' | arm-linux-gnueabihf-gcc -c -x assembler-with-cpp
>>>>>>> -
>>>>>>>
>>>>>>> and grepping for r12 under arch/arm suggests the same
>>>>>> The use of "r12" is fine. But it's not just the assembler,
>>>>>> I believe it also involves gcc.
>>>>>>
>>>>>> The problem is with the use of the __asmeq(x, y) macro.
>>>>>>
>>>>> Ah right. Apologies for assuming that you had missed something obvious
>>>>> here.
>>>>> But __asmeq is not the toolchain, it is a local construct #define'd in
>>>>> compiler.h
>>>>>
>>>>>> If I assign the "ip" variable with "r12":
>>>>>> register u32 ip asm("r12"); /* Also called ip */
>>>>>>
>>>>>> Then that's fine. However, this line then causes an error:
>>>>>> __asmeq("%0", "r12")
>>>>>>
>>>>>> Apparently gcc uses register "ip" when it sees asm("r12"). So
>>>>>> attempting to verify the desired register got used with __asmeq()
>>>>>> causes a string mismatch--"ip" is not equal to "r12".
>>>>>>
>>>>>> So I could use:
>>>>>>
>>>>>> register u32 ip asm("r12"); /* Also called ip */
>>>>>> ...
>>>>>> __asmeq("%0", &
Re: [PATCH] bcm: address clang inline asm incompatibility
On 01/28/15 11:38, Ard Biesheuvel wrote:
> On 28 January 2015 at 19:27, Alex Elder wrote:
>> On 01/28/2015 01:17 PM, Ard Biesheuvel wrote:
>>> On 28 January 2015 at 17:20, Ard Biesheuvel
>>> wrote:
>>>> On 28 January 2015 at 17:08, Alex Elder wrote:
>>>>> On 01/28/2015 10:17 AM, Ard Biesheuvel wrote:
>>>>>> On 28 January 2015 at 14:11, Alex Elder wrote:
>>>>>>> On 01/28/2015 05:15 AM, Ard Biesheuvel wrote:
>>>>>>>> On 28 January 2015 at 05:18, Behan Webster
>>>>>>>> wrote:
>>>>>>>>> From: Alex Elder
>>>>>>>>>
>>>>>>>>> My GCC-based build environment likes to call register r12 by the
>>>>>>>>> name "ip" in inline asm. Behan Webster informed me that his Clang-
>>>>>>>>> based build environment likes "r12" instead.
>>>>>>>>>
>>>>>>>>> Try to make them both happy.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Alex Elder
>>>>>>>>> Signed-off-by: Behan Webster
>>>>>>>>> ---
>>>>>>>>> arch/arm/mach-bcm/bcm_kona_smc.c | 9 +++--
>>>>>>>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/arm/mach-bcm/bcm_kona_smc.c
>>>>>>>>> b/arch/arm/mach-bcm/bcm_kona_smc.c
>>>>>>>>> index a55a7ec..3937bd5 100644
>>>>>>>>> --- a/arch/arm/mach-bcm/bcm_kona_smc.c
>>>>>>>>> +++ b/arch/arm/mach-bcm/bcm_kona_smc.c
>>>>>>>>> @@ -106,9 +106,14 @@ int __init bcm_kona_smc_init(void)
>>>>>>>>> * request result appropriately. This result value is found in r0
>>>>>>>>> * when the "smc" request completes.
>>>>>>>>> */
>>>>>>>>> +#ifdef __clang__
>>>>>>>>> +#define R12"r12"
>>>>>>>>> +#else /* !__clang__ */
>>>>>>>>> +#define R12"ip"/* gcc calls r12 "ip" */
>>>>>>>>> +#endif /* !__clang__ */
>>>>>>>> Why not just use r12 for both?
>>>>>>> Yes, that would have been an obvious fix. But the
>>>>>>> assembler (in the GCC environment) doesn't accept that.
>>>>>>>
>>>>>> Mine has no problems with it at all
>>>>>>
>>>>>> $ echo 'mov r12, #0' | arm-linux-gnueabihf-gcc -c -x assembler-with-cpp -
>>>>>>
>>>>>> and grepping for r12 under arch/arm suggests the same
>>>>> The use of "r12" is fine. But it's not just the assembler,
>>>>> I believe it also involves gcc.
>>>>>
>>>>> The problem is with the use of the __asmeq(x, y) macro.
>>>>>
>>>> Ah right. Apologies for assuming that you had missed something obvious
>>>> here.
>>>> But __asmeq is not the toolchain, it is a local construct #define'd in
>>>> compiler.h
>>>>
>>>>> If I assign the "ip" variable with "r12":
>>>>> register u32 ip asm("r12"); /* Also called ip */
>>>>>
>>>>> Then that's fine. However, this line then causes an error:
>>>>> __asmeq("%0", "r12")
>>>>>
>>>>> Apparently gcc uses register "ip" when it sees asm("r12"). So
>>>>> attempting to verify the desired register got used with __asmeq()
>>>>> causes a string mismatch--"ip" is not equal to "r12".
>>>>>
>>>>> So I could use:
>>>>>
>>>>> register u32 ip asm("r12"); /* Also called ip */
>>>>> ...
>>>>> __asmeq("%0", "ip")
>>>>>
>>>>> And that will build. But it's a little non-intuitive, and
>>>>> I suspect that clang might (rightfully) have a failure in
>>>>> this __asmeq() call.
>>>>>
>>>> In that case, I would strongly suggest fixing the __asmeq () macro
>>>> instead, and teach it that ("r12","ip") and ("ip","r12") are fine too.
>>>>
>>>> The thing is, inline asm is a dodgy area to begin with in terms of
>>>> clang-to-gcc compatibility. On arm64, we have been seeing issues where
>>>> the width of the register -which is fixed on gcc- is selected based on
>>>> the size of that variable, i.e., an int32 gets a w# register and int64
>>>> gets a x# register. Imagine debugging that, e.g., a str %0, [xx] that
>>>> writes 8 bytes on GCC suddenly only writing 4 bytes when built with
>>>> clang.
>>>>
>>>> If we also start using the preprocessor to conditionalise what is
>>>> emitted by inline asm, the waters get even murkier and it becomes even
>>>> harder to claim parity between the two.
>>>>
>>> Something like this perhaps?
>> So __asmeq() yields true if the register names (strings) are
>> equal, or if one is "ip" and the other is "r12" (in either order).
>>
>> I can't comment on whether it's right in all build environments but
>> this looks OK to me, to handle this special case.
>>
>> I would much rather you generate that patch. Is that OK?
>>
> Sure, I can cook up a patch if you guys can confirm that it fixes your
> use case. (I tested GCC myself but I don't have clang installed)
>
That appears to work with clang as well.
All in all a much better solution.
Thank you,
Behan
--
Behan Webster
beh...@converseincode.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] bcm: address clang inline asm incompatibility
On 01/28/15 11:17, Ard Biesheuvel wrote:
> On 28 January 2015 at 17:20, Ard Biesheuvel wrote:
>> On 28 January 2015 at 17:08, Alex Elder wrote:
>>> On 01/28/2015 10:17 AM, Ard Biesheuvel wrote:
>>>> On 28 January 2015 at 14:11, Alex Elder wrote:
>>>>> On 01/28/2015 05:15 AM, Ard Biesheuvel wrote:
>>>>>> On 28 January 2015 at 05:18, Behan Webster
>>>>>> wrote:
>>>>>>> From: Alex Elder
>>>>>>>
>>>>>>> My GCC-based build environment likes to call register r12 by the
>>>>>>> name "ip" in inline asm. Behan Webster informed me that his Clang-
>>>>>>> based build environment likes "r12" instead.
>>>>>>>
>>>>>>> Try to make them both happy.
>>>>>>>
>>>>>>> Signed-off-by: Alex Elder
>>>>>>> Signed-off-by: Behan Webster
>>>>>>> ---
>>>>>>> arch/arm/mach-bcm/bcm_kona_smc.c | 9 +++--
>>>>>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/arm/mach-bcm/bcm_kona_smc.c
>>>>>>> b/arch/arm/mach-bcm/bcm_kona_smc.c
>>>>>>> index a55a7ec..3937bd5 100644
>>>>>>> --- a/arch/arm/mach-bcm/bcm_kona_smc.c
>>>>>>> +++ b/arch/arm/mach-bcm/bcm_kona_smc.c
>>>>>>> @@ -106,9 +106,14 @@ int __init bcm_kona_smc_init(void)
>>>>>>> * request result appropriately. This result value is found in r0
>>>>>>> * when the "smc" request completes.
>>>>>>> */
>>>>>>> +#ifdef __clang__
>>>>>>> +#define R12"r12"
>>>>>>> +#else /* !__clang__ */
>>>>>>> +#define R12"ip"/* gcc calls r12 "ip" */
>>>>>>> +#endif /* !__clang__ */
>>>>>> Why not just use r12 for both?
>>>>> Yes, that would have been an obvious fix. But the
>>>>> assembler (in the GCC environment) doesn't accept that.
>>>>>
>>>> Mine has no problems with it at all
>>>>
>>>> $ echo 'mov r12, #0' | arm-linux-gnueabihf-gcc -c -x assembler-with-cpp -
>>>>
>>>> and grepping for r12 under arch/arm suggests the same
>>> The use of "r12" is fine. But it's not just the assembler,
>>> I believe it also involves gcc.
>>>
>>> The problem is with the use of the __asmeq(x, y) macro.
>>>
>> Ah right. Apologies for assuming that you had missed something obvious here.
>> But __asmeq is not the toolchain, it is a local construct #define'd in
>> compiler.h
>>
>>> If I assign the "ip" variable with "r12":
>>> register u32 ip asm("r12"); /* Also called ip */
>>>
>>> Then that's fine. However, this line then causes an error:
>>> __asmeq("%0", "r12")
>>>
>>> Apparently gcc uses register "ip" when it sees asm("r12"). So
>>> attempting to verify the desired register got used with __asmeq()
>>> causes a string mismatch--"ip" is not equal to "r12".
>>>
>>> So I could use:
>>>
>>> register u32 ip asm("r12"); /* Also called ip */
>>> ...
>>> __asmeq("%0", "ip")
>>>
>>> And that will build. But it's a little non-intuitive, and
>>> I suspect that clang might (rightfully) have a failure in
>>> this __asmeq() call.
>>>
>> In that case, I would strongly suggest fixing the __asmeq () macro
>> instead, and teach it that ("r12","ip") and ("ip","r12") are fine too.
>>
>> The thing is, inline asm is a dodgy area to begin with in terms of
>> clang-to-gcc compatibility. On arm64, we have been seeing issues where
>> the width of the register -which is fixed on gcc- is selected based on
>> the size of that variable, i.e., an int32 gets a w# register and int64
>> gets a x# register. Imagine debugging that, e.g., a str %0, [xx] that
>> writes 8 bytes on GCC suddenly only writing 4 bytes when built with
>> clang.
>>
>> If we also start using the preprocessor to conditionalise what is
>> emitted by inline asm, the waters get even murkier and it becomes even
>> harder to claim parity between the two.
>>
> Something like this perhaps?
>
> >8--
> diff --git a/arch/arm/include/asm/compiler.h b/arch/arm/include/asm/compiler.h
> index 8155db2f7fa1..f99c674b3751 100644
> --- a/arch/arm/include/asm/compiler.h
> +++ b/arch/arm/include/asm/compiler.h
> @@ -9,7 +9,8 @@
> * will cause compilation to stop on mismatch.
> * (for details, see gcc PR 15089)
> */
> -#define __asmeq(x, y) ".ifnc " x "," y " ; .err ; .endif\n\t"
> +#define __asmeq(x, y) ".ifnc " x "," y " ; .ifnc " x y ",ipr12 ; " \
> + ".ifnc " x y ",r12ip ; .err ; .endif ; .endif ; .endif\n\t"
>
>
> #endif /* __ASM_ARM_COMPILER_H */
> >8--
If that is acceptable, that's fine by me.
In principal none of us *want* to use #ifdefs.
Behan
--
Behan Webster
beh...@converseincode.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Mark inline functions as __maybe_unused
clang warns if inline functions aren't used. By making them __maybe_unused there is no warning for either gcc nor clang. Signed-off-by: Behan Webster Suggested-by: Arnd Bergmann Cc: Arnd Bergmann Cc: "Christopher Li" --- include/linux/compiler-gcc.h | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index 02ae99e..c7b98fb 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -53,14 +53,14 @@ */ #if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) || \ !defined(CONFIG_OPTIMIZE_INLINING) || (__GNUC__ < 4) -# define inlineinline __attribute__((always_inline)) notrace -# define __inline____inline__ __attribute__((always_inline)) notrace -# define __inline __inline__attribute__((always_inline)) notrace +# define inlineinline __attribute__((always_inline)) notrace __maybe_unused +# define __inline____inline__ __attribute__((always_inline)) notrace __maybe_unused +# define __inline __inline__attribute__((always_inline)) notrace __maybe_unused #else /* A lot of inline functions can cause havoc with function tracing */ -# define inlineinline notrace -# define __inline____inline__ notrace -# define __inline __inlinenotrace +# define inlineinline notrace __maybe_unused +# define __inline____inline__ notrace __maybe_unused +# define __inline __inlinenotrace __maybe_unused #endif #define __deprecated __attribute__((deprecated)) -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] bcm: address clang inline asm incompatibility
From: Alex Elder
My GCC-based build environment likes to call register r12 by the
name "ip" in inline asm. Behan Webster informed me that his Clang-
based build environment likes "r12" instead.
Try to make them both happy.
Signed-off-by: Alex Elder
Signed-off-by: Behan Webster
---
arch/arm/mach-bcm/bcm_kona_smc.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-bcm/bcm_kona_smc.c b/arch/arm/mach-bcm/bcm_kona_smc.c
index a55a7ec..3937bd5 100644
--- a/arch/arm/mach-bcm/bcm_kona_smc.c
+++ b/arch/arm/mach-bcm/bcm_kona_smc.c
@@ -106,9 +106,14 @@ int __init bcm_kona_smc_init(void)
* request result appropriately. This result value is found in r0
* when the "smc" request completes.
*/
+#ifdef __clang__
+#define R12"r12"
+#else /* !__clang__ */
+#define R12"ip"/* gcc calls r12 "ip" */
+#endif /* !__clang__ */
static int bcm_kona_do_smc(u32 service_id, u32 buffer_phys)
{
- register u32 ip asm("ip"); /* Also called r12 */
+ register u32 ip asm(R12); /* Also called r12 */
register u32 r0 asm("r0");
register u32 r4 asm("r4");
register u32 r5 asm("r5");
@@ -120,7 +125,7 @@ static int bcm_kona_do_smc(u32 service_id, u32 buffer_phys)
asm volatile (
/* Make sure we got the registers we want */
- __asmeq("%0", "ip")
+ __asmeq("%0", R12)
__asmeq("%1", "r0")
__asmeq("%2", "r4")
__asmeq("%3", "r5")
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 4/4] staging, rtl8192e, LLVMLinux: Make static local in inline function const
rtllib_association_req is a (large) inline function which defines 2 constant
static arrays which aren't labelled as const. As a result clang complains with:
non-constant static local variable in inline function may be different in
different files
[-Wstatic-local-in-inline]
static u8 AironetIeOui[] = {0x00, 0x01, 0x66};
^
The solution is making them "static const".
However doing so requires dropping const when being used with struct
octet_string. However the value is used in a const fashion thereafter, so no
harm done.
Signed-off-by: Behan Webster
Suggested-by: Arnd Bergmann
Cc: Arnd Bergmann
---
drivers/staging/rtl8192e/rtllib_softmac.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/rtl8192e/rtllib_softmac.c
b/drivers/staging/rtl8192e/rtllib_softmac.c
index 089a058..e970db4 100644
--- a/drivers/staging/rtl8192e/rtllib_softmac.c
+++ b/drivers/staging/rtl8192e/rtllib_softmac.c
@@ -1311,7 +1311,7 @@ inline struct sk_buff *rtllib_association_req(struct
rtllib_network *beacon,
}
if (beacon->bCkipSupported) {
- static u8 AironetIeOui[] = {0x00, 0x01, 0x66};
+ static const u8 AironetIeOui[] = {0x00, 0x01, 0x66};
u8 CcxAironetBuf[30];
struct octet_string osCcxAironetIE;
@@ -1331,10 +1331,11 @@ inline struct sk_buff *rtllib_association_req(struct
rtllib_network *beacon,
}
if (beacon->bCcxRmEnable) {
- static u8 CcxRmCapBuf[] = {0x00, 0x40, 0x96, 0x01, 0x01, 0x00};
+ static const u8 CcxRmCapBuf[] = {0x00, 0x40, 0x96, 0x01, 0x01,
+ 0x00};
struct octet_string osCcxRmCap;
- osCcxRmCap.Octet = CcxRmCapBuf;
+ osCcxRmCap.Octet = (u8 *) CcxRmCapBuf;
osCcxRmCap.Length = sizeof(CcxRmCapBuf);
tag = skb_put(skb, ccxrm_ie_len);
*tag++ = MFIE_TYPE_GENERIC;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/4] staging, rtl8192e, LLVMLinux: Remove unused inline prototype
rtllib_probe_req is defined as "static inline" in rtllib_softmac.c however it is declared differently as "extern inline" in rtllib_softmac.h. Since it isn't used outside of the scope of rtllib_softmac, it makes sense to remove the incorrect declaration. Signed-off-by: Behan Webster Suggested-by: Arnd Bergmann Cc: Arnd Bergmann --- drivers/staging/rtl8192e/rtllib.h | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/staging/rtl8192e/rtllib.h b/drivers/staging/rtl8192e/rtllib.h index 33995ac..1322782 100644 --- a/drivers/staging/rtl8192e/rtllib.h +++ b/drivers/staging/rtl8192e/rtllib.h @@ -2762,7 +2762,6 @@ extern void rtllib_stop_scan(struct rtllib_device *ieee); extern bool rtllib_act_scanning(struct rtllib_device *ieee, bool sync_scan); extern void rtllib_stop_scan_syncro(struct rtllib_device *ieee); extern void rtllib_start_scan_syncro(struct rtllib_device *ieee, u8 is_mesh); -extern inline struct sk_buff *rtllib_probe_req(struct rtllib_device *ieee); extern u8 MgntQuery_MgntFrameTxRate(struct rtllib_device *ieee); extern void rtllib_sta_ps_send_null_frame(struct rtllib_device *ieee, short pwr); -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/4] staging, rtl8192e, LLVMLinux: Remove unused prototype
MgntQuery_MgntFrameTxRate is only used within rtllib_softmac.c, so it really
should be static instead of extern.
Since it is currently extern a warning is generated because a different
function of the same name is defined staticlly in ieee80211_softmac.c
Removing the incorrect extern declaration and defining the rtllib_softmac
version of this routine static fixes the warning.
Signed-off-by: Behan Webster
Suggested-by: Arnd Bergmann
Cc: Arnd Bergmann
---
drivers/staging/rtl8192e/rtllib.h | 1 -
drivers/staging/rtl8192e/rtllib_softmac.c | 2 +-
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/rtl8192e/rtllib.h
b/drivers/staging/rtl8192e/rtllib.h
index 1322782..cef2dc2 100644
--- a/drivers/staging/rtl8192e/rtllib.h
+++ b/drivers/staging/rtl8192e/rtllib.h
@@ -2762,7 +2762,6 @@ extern void rtllib_stop_scan(struct rtllib_device *ieee);
extern bool rtllib_act_scanning(struct rtllib_device *ieee, bool sync_scan);
extern void rtllib_stop_scan_syncro(struct rtllib_device *ieee);
extern void rtllib_start_scan_syncro(struct rtllib_device *ieee, u8 is_mesh);
-extern u8 MgntQuery_MgntFrameTxRate(struct rtllib_device *ieee);
extern void rtllib_sta_ps_send_null_frame(struct rtllib_device *ieee,
short pwr);
extern void rtllib_sta_wakeup(struct rtllib_device *ieee, short nl);
diff --git a/drivers/staging/rtl8192e/rtllib_softmac.c
b/drivers/staging/rtl8192e/rtllib_softmac.c
index 067a45a..089a058 100644
--- a/drivers/staging/rtl8192e/rtllib_softmac.c
+++ b/drivers/staging/rtl8192e/rtllib_softmac.c
@@ -193,7 +193,7 @@ MgntQuery_TxRateExcludeCCKRates(struct rtllib_device *ieee)
return QueryRate;
}
-u8 MgntQuery_MgntFrameTxRate(struct rtllib_device *ieee)
+static u8 MgntQuery_MgntFrameTxRate(struct rtllib_device *ieee)
{
struct rt_hi_throughput *pHTInfo = ieee->pHTInfo;
u8 rate;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/4] staging, rtl8192e, LLVMLinux: Patches to enable the kernel to be compiled with clang/LLVM
Removing a number of warnings generated from compiling stl8192e with clang. The LLVMLinux project aims to fully build the Linux kernel using both gcc and clang (the C front end for the LLVM compiler infrastructure project). Behan Webster (4): staging, rtl8192e, LLVMLinux: Change extern inline to static inline staging, rtl8192e, LLVMLinux: Remove unused inline prototype staging, rtl8192e, LLVMLinux: Remove unused prototype staging, rtl8192e, LLVMLinux: Make static local in inline function const drivers/staging/rtl8192e/rtllib.h | 6 ++ drivers/staging/rtl8192e/rtllib_softmac.c | 11 ++- 2 files changed, 8 insertions(+), 9 deletions(-) -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/4] staging, rtl8192e, LLVMLinux: Change extern inline to static inline
With compilers which follow the C99 standard (like modern versions of gcc and
clang), "extern inline" does the opposite thing from older versions of gcc
(emits code for an externally linkable version of the inline function).
"static inline" does the intended behavior in all cases instead.
Signed-off-by: Behan Webster
Suggested-by: Arnd Bergmann
Cc: Arnd Bergmann
---
drivers/staging/rtl8192e/rtllib.h | 4 ++--
drivers/staging/rtl8192e/rtllib_softmac.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/rtl8192e/rtllib.h
b/drivers/staging/rtl8192e/rtllib.h
index 2d82f89..33995ac 100644
--- a/drivers/staging/rtl8192e/rtllib.h
+++ b/drivers/staging/rtl8192e/rtllib.h
@@ -2944,12 +2944,12 @@ void rtllib_softmac_scan_syncro(struct rtllib_device
*ieee, u8 is_mesh);
extern const long rtllib_wlan_frequencies[];
-extern inline void rtllib_increment_scans(struct rtllib_device *ieee)
+static inline void rtllib_increment_scans(struct rtllib_device *ieee)
{
ieee->scans++;
}
-extern inline int rtllib_get_scans(struct rtllib_device *ieee)
+static inline int rtllib_get_scans(struct rtllib_device *ieee)
{
return ieee->scans;
}
diff --git a/drivers/staging/rtl8192e/rtllib_softmac.c
b/drivers/staging/rtl8192e/rtllib_softmac.c
index abb6729..067a45a 100644
--- a/drivers/staging/rtl8192e/rtllib_softmac.c
+++ b/drivers/staging/rtl8192e/rtllib_softmac.c
@@ -343,7 +343,7 @@ inline void softmac_ps_mgmt_xmit(struct sk_buff *skb,
}
}
-inline struct sk_buff *rtllib_probe_req(struct rtllib_device *ieee)
+static inline struct sk_buff *rtllib_probe_req(struct rtllib_device *ieee)
{
unsigned int len, rate_len;
u8 *tag;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] staging, rts5208, LLVMLinux: Change extern inline to static inline
With compilers which follow the C99 standard (like modern versions of gcc and
clang), "extern inline" does the opposite thing from older versions of gcc
(emits code for an externally linkable version of the inline function). "static
inline" does the intended behavior in both gcc and clang.
Signed-off-by: Behan Webster
Suggested-by: Arnd Bergmann
Cc: Arnd Bergmann
---
drivers/staging/rts5208/rtsx_transport.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/rts5208/rtsx_transport.h
b/drivers/staging/rts5208/rtsx_transport.h
index b4b1123..899bc20 100644
--- a/drivers/staging/rts5208/rtsx_transport.h
+++ b/drivers/staging/rts5208/rtsx_transport.h
@@ -46,7 +46,7 @@ void rtsx_add_cmd(struct rtsx_chip *chip,
void rtsx_send_cmd_no_wait(struct rtsx_chip *chip);
int rtsx_send_cmd(struct rtsx_chip *chip, u8 card, int timeout);
-extern inline u8 *rtsx_get_cmd_data(struct rtsx_chip *chip)
+static inline u8 *rtsx_get_cmd_data(struct rtsx_chip *chip)
{
#ifdef CMD_USING_SG
return (u8 *)(chip->host_sg_tbl_ptr);
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[GIT PULL] LLVMLinux patches for v3.18
These patches remove the use of VLAIS using a new SHASH_DESC_ON_STACK macro. Some of the previously accepted VLAIS removal patches haven't used this macro. I will push new patches to consistently use this macro in all those older cases for 3.19 The following changes since commit 2d65a9f48fcdf7866aab6457bc707ca233e0c791: Merge branch 'drm-next' of git://people.freedesktop.org/~airlied/linux (2014-10-14 09:39:08 +0200) are available in the git repository at: git://git.linuxfoundation.org/llvmlinux/kernel.git tags/llvmlinux-for-v3.18 for you to fetch changes up to 4c5c30249452aaebf258751ea4222eba3dd3da4c: crypto: LLVMLinux: Remove VLAIS usage from crypto/testmgr.c (2014-10-14 10:51:24 +0200) LLVMLinux patches for v3.18 -------- Behan Webster (6): crypto: LLVMLinux: Add macro to remove use of VLAIS in crypto code crypto: LLVMLinux: Remove VLAIS from crypto/mv_cesa.c crypto: LLVMLinux: Remove VLAIS from crypto/n2_core.c crypto: LLVMLinux: Remove VLAIS from crypto/omap_sham.c crypto: LLVMLinux: Remove VLAIS from crypto/.../qat_algs.c security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c Jan-Simon Möller (5): crypto: LLVMLinux: Remove VLAIS from crypto/ccp/ccp-crypto-sha.c crypto, dm: LLVMLinux: Remove VLAIS usage from dm-crypt crypto: LLVMLinux: Remove VLAIS usage from crypto/hmac.c crypto: LLVMLinux: Remove VLAIS usage from libcrc32c.c crypto: LLVMLinux: Remove VLAIS usage from crypto/testmgr.c Vinícius Tinti (1): btrfs: LLVMLinux: Remove VLAIS crypto/hmac.c| 25 - crypto/testmgr.c | 14 -- drivers/crypto/ccp/ccp-crypto-sha.c | 13 - drivers/crypto/mv_cesa.c | 41 drivers/crypto/n2_core.c | 11 +++- drivers/crypto/omap-sham.c | 28 --- drivers/crypto/qat/qat_common/qat_algs.c | 31 ++--- drivers/md/dm-crypt.c| 34 ++- fs/btrfs/hash.c | 16 +-- include/crypto/hash.h| 5 lib/libcrc32c.c | 16 +-- security/integrity/ima/ima_crypto.c | 47 +--- 12 files changed, 122 insertions(+), 159 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] arm, fbdev, omap2, LLVMLinux: Remove nested function from omap2 dss
On 09/27/14 09:46, Felipe Balbi wrote: On Fri, Sep 26, 2014 at 06:10:52PM -0700, Behan Webster wrote: Replace the use of nested functions where a normal function will suffice. Nested functions are not liked by upstream kernel developers in general. Their use breaks the use of clang as a compiler, and doesn't make the code any better. This code now works for both gcc and clang. Signed-off-by: Behan Webster Suggested-by: Arnd Bergmann Cc: Arnd Bergmann another one that make sense :-) And probably also deserves checkpatch/coccinelle/sparse. Indeed! That would be very appreciated. The clang static analyzer already points this one out. :) Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] media, platform, LLVMLinux: Remove nested function from ti-vpe
Replace the use of nested functions where a normal function will suffice.
Nested functions are not liked by upstream kernel developers in general. Their
use breaks the use of clang as a compiler, and doesn't make the code any
better.
This code now works for both gcc and clang.
Signed-off-by: Behan Webster
Suggested-by: Arnd Bergmann
Cc: Arnd Bergmann
---
drivers/media/platform/ti-vpe/csc.c | 8 ++--
drivers/media/platform/ti-vpe/sc.c | 8 ++--
2 files changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/media/platform/ti-vpe/csc.c
b/drivers/media/platform/ti-vpe/csc.c
index 940df40..44fbf41 100644
--- a/drivers/media/platform/ti-vpe/csc.c
+++ b/drivers/media/platform/ti-vpe/csc.c
@@ -93,12 +93,8 @@ void csc_dump_regs(struct csc_data *csc)
{
struct device *dev = &csc->pdev->dev;
- u32 read_reg(struct csc_data *csc, int offset)
- {
- return ioread32(csc->base + offset);
- }
-
-#define DUMPREG(r) dev_dbg(dev, "%-35s %08x\n", #r, read_reg(csc, CSC_##r))
+#define DUMPREG(r) dev_dbg(dev, "%-35s %08x\n", #r, \
+ ioread32(csc->base + CSC_##r))
DUMPREG(CSC00);
DUMPREG(CSC01);
diff --git a/drivers/media/platform/ti-vpe/sc.c
b/drivers/media/platform/ti-vpe/sc.c
index 6314171..1088381 100644
--- a/drivers/media/platform/ti-vpe/sc.c
+++ b/drivers/media/platform/ti-vpe/sc.c
@@ -24,12 +24,8 @@ void sc_dump_regs(struct sc_data *sc)
{
struct device *dev = &sc->pdev->dev;
- u32 read_reg(struct sc_data *sc, int offset)
- {
- return ioread32(sc->base + offset);
- }
-
-#define DUMPREG(r) dev_dbg(dev, "%-35s %08x\n", #r, read_reg(sc, CFG_##r))
+#define DUMPREG(r) dev_dbg(dev, "%-35s %08x\n", #r, \
+ ioread32(sc->base + CFG_##r))
DUMPREG(SC0);
DUMPREG(SC1);
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] arm, fbdev, omap2, LLVMLinux: Remove nested function from omapfb
Replace the use of nested functions where a normal function will suffice.
Nested functions are not liked by upstream kernel developers in general. Their
use breaks the use of clang as a compiler, and doesn't make the code any
better.
This code now works for both gcc and clang.
Signed-off-by: Behan Webster
Suggested-by: Arnd Bergmann
Cc: Arnd Bergmann
---
drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
index ec2d132..1587243 100644
--- a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
+++ b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
@@ -273,16 +273,16 @@ static struct omapfb_colormode omapfb_colormodes[] = {
},
};
+static bool cmp_component(struct fb_bitfield *f1, struct fb_bitfield *f2)
+{
+ return f1->length == f2->length &&
+ f1->offset == f2->offset &&
+ f1->msb_right == f2->msb_right;
+}
+
static bool cmp_var_to_colormode(struct fb_var_screeninfo *var,
struct omapfb_colormode *color)
{
- bool cmp_component(struct fb_bitfield *f1, struct fb_bitfield *f2)
- {
- return f1->length == f2->length &&
- f1->offset == f2->offset &&
- f1->msb_right == f2->msb_right;
- }
-
if (var->bits_per_pixel == 0 ||
var->red.length == 0 ||
var->blue.length == 0 ||
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/2] LLVMLinux: Patches to enable the kernel to be compiled with clang/LLVM
Replace the use of nested functions where a normal function will suffice. Nested functions are not liked by upstream kernel developers in general. Their use breaks the use of clang as a compiler, and doesn't make the code any better. This code now works for both gcc and clang. The LLVMLinux project aims to fully build the Linux kernel using both gcc and clang (the C front end for the LLVM compiler infrastructure project). Behan Webster (2): arm, fbdev, omap2, LLVMLinux: Remove nested function from omap2 dss arm, fbdev, omap2, LLVMLinux: Remove nested function from omapfb drivers/video/fbdev/omap2/dss/dispc-compat.c | 9 + drivers/video/fbdev/omap2/dss/manager-sysfs.c | 16 +--- drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 14 +++--- 3 files changed, 21 insertions(+), 18 deletions(-) -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] arm, fbdev, omap2, LLVMLinux: Remove nested function from omap2 dss
Replace the use of nested functions where a normal function will suffice.
Nested functions are not liked by upstream kernel developers in general. Their
use breaks the use of clang as a compiler, and doesn't make the code any
better.
This code now works for both gcc and clang.
Signed-off-by: Behan Webster
Suggested-by: Arnd Bergmann
Cc: Arnd Bergmann
---
drivers/video/fbdev/omap2/dss/dispc-compat.c | 9 +
drivers/video/fbdev/omap2/dss/manager-sysfs.c | 16 +---
2 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/drivers/video/fbdev/omap2/dss/dispc-compat.c
b/drivers/video/fbdev/omap2/dss/dispc-compat.c
index 83779c2..633c461 100644
--- a/drivers/video/fbdev/omap2/dss/dispc-compat.c
+++ b/drivers/video/fbdev/omap2/dss/dispc-compat.c
@@ -634,13 +634,14 @@ void dispc_mgr_disable_sync(enum omap_channel channel)
WARN_ON(1);
}
+static inline void dispc_irq_wait_handler(void *data, u32 mask)
+{
+ complete((struct completion *)data);
+}
+
int omap_dispc_wait_for_irq_interruptible_timeout(u32 irqmask,
unsigned long timeout)
{
- void dispc_irq_wait_handler(void *data, u32 mask)
- {
- complete((struct completion *)data);
- }
int r;
DECLARE_COMPLETION_ONSTACK(completion);
diff --git a/drivers/video/fbdev/omap2/dss/manager-sysfs.c
b/drivers/video/fbdev/omap2/dss/manager-sysfs.c
index 37b59fe..a7414fb 100644
--- a/drivers/video/fbdev/omap2/dss/manager-sysfs.c
+++ b/drivers/video/fbdev/omap2/dss/manager-sysfs.c
@@ -44,6 +44,13 @@ static ssize_t manager_display_show(struct
omap_overlay_manager *mgr, char *buf)
dssdev->name : "");
}
+static int manager_display_match(struct omap_dss_device *dssdev, void *data)
+{
+ const char *str = data;
+
+ return sysfs_streq(dssdev->name, str);
+}
+
static ssize_t manager_display_store(struct omap_overlay_manager *mgr,
const char *buf, size_t size)
{
@@ -52,17 +59,12 @@ static ssize_t manager_display_store(struct
omap_overlay_manager *mgr,
struct omap_dss_device *dssdev = NULL;
struct omap_dss_device *old_dssdev;
- int match(struct omap_dss_device *dssdev, void *data)
- {
- const char *str = data;
- return sysfs_streq(dssdev->name, str);
- }
-
if (buf[size-1] == '\n')
--len;
if (len > 0)
- dssdev = omap_dss_find_device((void *)buf, match);
+ dssdev = omap_dss_find_device((void *)buf,
+ manager_display_match);
if (len > 0 && dssdev == NULL)
return -EINVAL;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] md, sysfs, LLVMLinux: Remove nested function from bcache sysfs
Replace the use of nested functions where a normal function will suffice.
Nested functions are not liked by upstream kernel developers in general. Their
use breaks the use of clang as a compiler, and doesn't make the code any
better.
This code now works for both gcc and clang.
Signed-off-by: Behan Webster
Suggested-by: Arnd Bergmann
Cc: Arnd Bergmann
---
drivers/md/bcache/sysfs.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
index b3ff57d..53d8baa 100644
--- a/drivers/md/bcache/sysfs.c
+++ b/drivers/md/bcache/sysfs.c
@@ -731,6 +731,11 @@ static struct attribute *bch_cache_set_internal_files[] = {
};
KTYPE(bch_cache_set_internal);
+static int __bch_cache_cmp(const void *l, const void *r)
+{
+ return *((uint16_t *) r) - *((uint16_t *) l);
+}
+
SHOW(__bch_cache)
{
struct cache *ca = container_of(kobj, struct cache, kobj);
@@ -755,9 +760,6 @@ SHOW(__bch_cache)
CACHE_REPLACEMENT(&ca->sb));
if (attr == &sysfs_priority_stats) {
- int cmp(const void *l, const void *r)
- { return *((uint16_t *) r) - *((uint16_t *) l); }
-
struct bucket *b;
size_t n = ca->sb.nbuckets, i;
size_t unused = 0, available = 0, dirty = 0, meta = 0;
@@ -786,7 +788,7 @@ SHOW(__bch_cache)
p[i] = ca->buckets[i].prio;
mutex_unlock(&ca->set->bucket_lock);
- sort(p, n, sizeof(uint16_t), cmp, NULL);
+ sort(p, n, sizeof(uint16_t), __bch_cache_cmp, NULL);
while (n &&
!cached[n - 1])
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] clk, ti, LLVMLinux: Move __init outside of type definition
On 09/26/14 17:55, Felipe Balbi wrote:
On Fri, Sep 26, 2014 at 05:31:48PM -0700, Behan Webster wrote:
As written, the __init for ti_clk_get_div_table is in the middle of the return
type.
The gcc documentation indicates that section attributes should be added to the
end of the function declaration:
extern void foobar (void) __attribute__ ((section ("bar")));
However gcc seems to be very permissive with where attributes can be placed.
clang on the other hand isn't so permissive, and fails if you put the section
definition in the middle of the return type:
drivers/clk/ti/divider.c:298:28: error: expected ';' after struct
static struct clk_div_table
^
;
drivers/clk/ti/divider.c:298:1: warning: 'static' ignored on this
declaration [-Wmissing-declarations]
static struct clk_div_table
^
drivers/clk/ti/divider.c:299:9: error: type specifier missing,
defaults to 'int' [-Werror,-Wimplicit-int]
__init *ti_clk_get_div_table(struct device_node *node)
~~ ^
drivers/clk/ti/divider.c:345:9: warning: incompatible pointer types
returning 'struct clk_div_table *' from a function with result type 'int
*' [-Wincompatible-pointer-types]
return table;
^
drivers/clk/ti/divider.c:419:9: warning: incompatible pointer types
assigning to 'const struct clk_div_table *' from 'int *'
[-Wincompatible-pointer-types]
*table = ti_clk_get_div_table(node);
^ ~~
3 warnings and 2 errors generated.
By convention, most of the kernel code puts section attributes between the
return type and function name. In the case where the return type is a pointer,
it's important to place the '*' on left of the __init.
This updated code works for both gcc and clang.
Signed-off-by: Behan Webster
Reviewed-by: Mark Charlebois
makes sense to me:
Reviewed-by: Felipe Balbi
Thank you.
I wonder if we should add this a Sparse or Coccinelle rule.
+1
I'm hoping it can be added to checkpatch as well.
Behan
--
Behan Webster
beh...@converseincode.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] clk, ti, LLVMLinux: Move __init outside of type definition
As written, the __init for ti_clk_get_div_table is in the middle of the return
type.
The gcc documentation indicates that section attributes should be added to the
end of the function declaration:
extern void foobar (void) __attribute__ ((section ("bar")));
However gcc seems to be very permissive with where attributes can be placed.
clang on the other hand isn't so permissive, and fails if you put the section
definition in the middle of the return type:
drivers/clk/ti/divider.c:298:28: error: expected ';' after struct
static struct clk_div_table
^
;
drivers/clk/ti/divider.c:298:1: warning: 'static' ignored on this
declaration [-Wmissing-declarations]
static struct clk_div_table
^
drivers/clk/ti/divider.c:299:9: error: type specifier missing,
defaults to 'int' [-Werror,-Wimplicit-int]
__init *ti_clk_get_div_table(struct device_node *node)
~~ ^
drivers/clk/ti/divider.c:345:9: warning: incompatible pointer types
returning 'struct clk_div_table *' from a function with result type 'int
*' [-Wincompatible-pointer-types]
return table;
^
drivers/clk/ti/divider.c:419:9: warning: incompatible pointer types
assigning to 'const struct clk_div_table *' from 'int *'
[-Wincompatible-pointer-types]
*table = ti_clk_get_div_table(node);
^ ~~
3 warnings and 2 errors generated.
By convention, most of the kernel code puts section attributes between the
return type and function name. In the case where the return type is a pointer,
it's important to place the '*' on left of the __init.
This updated code works for both gcc and clang.
Signed-off-by: Behan Webster
Reviewed-by: Mark Charlebois
---
drivers/clk/ti/divider.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/ti/divider.c b/drivers/clk/ti/divider.c
index a837f70..bff2b5b 100644
--- a/drivers/clk/ti/divider.c
+++ b/drivers/clk/ti/divider.c
@@ -300,8 +300,8 @@ static struct clk *_register_divider(struct device *dev,
const char *name,
return clk;
}
-static struct clk_div_table
-__init *ti_clk_get_div_table(struct device_node *node)
+static struct clk_div_table *
+__init ti_clk_get_div_table(struct device_node *node)
{
struct clk_div_table *table;
const __be32 *divspec;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kbuild, LLVMLinux: Add -Werror to cc-option to support clang
On 09/25/14 06:34, Michal Marek wrote: On 2014-09-24 20:50, Behan Webster wrote: Getting clang to error on unused flags wasn't trivial (this change broke a lot of builds apparently). Fortunately we weren't the only ones who wanted it to behave like gcc in this case. I think it's going to be *much* harder to do the same for warnings. The argument given by supporters of the current situation is that if a warning isn't supported, why break the build? *sigh* I guess the reason to accept unknown warnings opentions is compatibility with Makefiles with hardcoded gcc-isms. BTW, GCC at some point started to ignore unknown -Wno-* options, for everyone's good of course. That's why we ended up with the cc-disable-warning function. If -W* options for clang need special care, then it might be a good idea to introduce cc-warning with the conditional -Werror for clang. There are not that many places where we add warnings, so the patch would be still short. That way, the possible silent failure is limited only to warning options with clang, which is not such a big deal. I'll try this approach. Thanks, Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3] mmc, sdhci, bcm-kona, LLVMLinux: Remove use of __initconst
The __initconst is in the wrong place, and when moved to the correct place
it uncovers an error where the variable is used by non-init data structures.
Instead merely make them const and put the const in the right spot.
Signed-off-by: Behan Webster
Reviewed-by: Mark Charlebois
Acked-by: Arnd Bergmann
Acked-by: Matt Porter
---
drivers/mmc/host/sdhci-bcm-kona.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mmc/host/sdhci-bcm-kona.c
b/drivers/mmc/host/sdhci-bcm-kona.c
index dd780c3..d085dfe 100644
--- a/drivers/mmc/host/sdhci-bcm-kona.c
+++ b/drivers/mmc/host/sdhci-bcm-kona.c
@@ -225,7 +225,7 @@ static struct sdhci_pltfm_data sdhci_pltfm_data_kona = {
SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
};
-static struct __initconst of_device_id sdhci_bcm_kona_of_match[] = {
+static const struct of_device_id sdhci_bcm_kona_of_match[] = {
{ .compatible = "brcm,kona-sdhci"},
{ .compatible = "bcm,kona-sdhci"}, /* deprecated name */
{}
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: How to build the kernel with Clang?
On 09/18/14 21:10, Masahiro Yamada wrote: Hi Clang folks, I'd like to know the status of Clang support in the Linux mainline. Still a work in progress. You still need to use the LLVMLinux patches on top of mainline to get it to work. We're upstreaming those patches as fast as we can. I can see some "clang" specific parts in makefiles, so I guess Clang is already supported to a certain extent. Some of the required parts are there. But mainline currently won't compile out of the box with clang. I just tried to build with "HOSTCC=clang CC=clang" but it would not work. Is there any tips I am missing here? Try building from our kernel repo listed on http://llvm.linuxfoundation.org Thanks, Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kbuild, LLVMLinux: Add -Werror to cc-option to support clang
On 09/24/14 05:07, Michal Marek wrote: On 2014-09-23 21:28, beh...@converseincode.com wrote: From: Mark Charlebois Clang will warn about unknown warnings but will not return false You mean unknown options, right? 2 kinds of options: flags and warnings. clang used to merely warn about unused/unsupported flags/warnings. It now returns errors for unknown flags, but not warnings (unless you specify -Werror). The issue is that a lot of existing projects which use clang expect the former behaviour (I agree that makes no sense, but there you go). unless -Werror is set. GCC will return false if an unknown warning is passed. Adding -Werror make both compiler behave the same. Can you please limit it to the clang case? Add an internal variable that either contains -Werror or nothing, depending on the compiler. I can do that. Will fix. What I fear is that if we use -Werror unconditionally and the user (or some automated build system) decides to add some silly option to KCFLAGS, we will get silent failures in the cc-option tests. A valid concern for sure. BTW, is there a chance that this would be fixed in some later clang version? Accepting unknown commandline options is a rather unusual behavior. How are all the ./configure scripts going to cope with it? Again, clang does error out on unknown compiler flags (as opposed to warnings). Getting clang to error on unused flags wasn't trivial (this change broke a lot of builds apparently). Fortunately we weren't the only ones who wanted it to behave like gcc in this case. I think it's going to be *much* harder to do the same for warnings. The argument given by supporters of the current situation is that if a warning isn't supported, why break the build? *sigh* The LLVMLinux project is pushing hard to fix these sorts of things in clang, but some changes are more likely than others. Thanks, Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kbuild, LLVMLinux: Fix asm-offset generation to work with clang
On 09/24/14 03:37, Arnd Bergmann wrote:
On Tuesday 23 September 2014 12:25:31 beh...@converseincode.com wrote:
#define DEFINE(sym, val) \
-asm volatile("\n->" #sym " %0 " #val : : "i" (val))
+ asm volatile("\n@->" #sym " %0 " #val : : "i" (val))
Isn't the '@' character to start a comment architecture specific?
I had worried about that as well (as we discussed at LCU), but with the
limited testing I tried this with it seemed to indicate that gas was
smart enough to handle it in most cases. However the kbuild test robot
indicates that this patch breaks MIPS. So indeed your concern is justified.
Gotta love the kbuild test robot. Thank you Fengguang Wu!
If this makes it work on ARM, what about other architectures?
For now, I think I need to try something else.
Thanks,
Behan
--
Behan Webster
beh...@converseincode.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] mmc, sdhci, bcm-kona, LLVMLinux: Remove use of __initconst
On 09/24/14 02:22, Arnd Bergmann wrote:
On Tuesday 23 September 2014 15:55:08 beh...@converseincode.com wrote:
--- a/drivers/mmc/host/sdhci-bcm-kona.c
+++ b/drivers/mmc/host/sdhci-bcm-kona.c
@@ -225,7 +225,7 @@ static struct sdhci_pltfm_data sdhci_pltfm_data_kona = {
SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
};
-static struct __initconst of_device_id sdhci_bcm_kona_of_match[] = {
+static struct of_device_id const sdhci_bcm_kona_of_match[] = {
{ .compatible = "brcm,kona-sdhci"},
{ .compatible = "bcm,kona-sdhci"}, /* deprecated name */
{}
Sorry for giving you trouble over such a simple patch (especially
one that I have acked already), but I just noticed that this is
not following the common style we use in the kernel.
It's all good. It's not like you haven't saved me a tonne of time
already! :)
Almost everywhere in Linux, we use
static const struct of_device_id sdhci_bcm_kona_of_match[] = {
not
static struct of_device_id const sdhci_bcm_kona_of_match[] = {
True enough. I put the const where I did to be in keeping with the
intent of __initconst, making the array const instead of the contained type.
AFAICT they behave in identical ways,
Indeed. For C in both cases the resulting array of struct of_device_id
ends up in .rodata, so functionally equivalent.
but the first one seems
easier to read for someone familiar with kernel code.
No worries. Happy to post a v3.
Linus Walleij: Would you like me to respin the "gpio, bcm-kona,
LLVMLinux: Remove use of __initconst" patch as well?
Behan
--
Behan Webster
beh...@converseincode.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] arm, vt8500, LLVMLlinux: Use mcr instead of mcr% for mach-vt8500
On 09/24/14 02:16, Arnd Bergmann wrote:
On Tuesday 23 September 2014 20:44:44 Behan Webster wrote:
The ASM below does not compile with clang and is not the way that the mcr
command is used in other parts of the kernel.
arch/arm/mach-vt8500/vt8500.c:72:11: error: invalid % escape in inline assembly
string
asm("mcr%? p15, 0, %0, c7, c0, 4" : : "r" (0));
~^~~~
1 error generated.
There are other forms that are supported on different ARM instruction sets but
generally the kernel just uses mcr as it is supported in all ARM instruction
sets.
Just for confirm: both forms are actually correct and we don't need this
backported for stable, right?
My understanding is that the %? carries a condition code to the next
instruction (which in this case is then ignored). So essentially in this
situation both are equivalent.
Signed-off-by: Behan Webster
Reviewed-by: Mark Charlebois
Acked-by: Will Deacon
Acked-by: Arnd Bergmann
Tony, would you like to pick this one up and send it in a pull request
to arm-soc, or should we apply it to fixes-non-critical directly?
Thanks,
Behan
--
Behan Webster
beh...@converseincode.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] fs, LLVMLinux: Remove warning from COMPATIBLE_IOCTL
From: Mark Charlebois cmd in COMPATIBLE_IOCTL is always a u32, so cast it so there isn't a warning about an overflow in XFORM. Signed-off-by: Mark Charlebois Signed-off-by: Behan Webster Acked-by: Arnd Bergmann --- fs/compat_ioctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c index afec645..f6ce1aa 100644 --- a/fs/compat_ioctl.c +++ b/fs/compat_ioctl.c @@ -810,7 +810,7 @@ static int compat_ioctl_preallocate(struct file *file, */ #define XFORM(i) (((i) ^ ((i) << 27) ^ ((i) << 17)) & 0x) -#define COMPATIBLE_IOCTL(cmd) XFORM(cmd), +#define COMPATIBLE_IOCTL(cmd) XFORM((u32)cmd), /* ioctl should not be warned about even if it's not implemented. Valid reasons to use this: - It is implemented with ->compat_ioctl on some device, but programs -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] arm, vt8500, LLVMLlinux: Use mcr instead of mcr% for mach-vt8500
The ASM below does not compile with clang and is not the way that the mcr
command is used in other parts of the kernel.
arch/arm/mach-vt8500/vt8500.c:72:11: error: invalid % escape in inline assembly
string
asm("mcr%? p15, 0, %0, c7, c0, 4" : : "r" (0));
~^~~~
1 error generated.
There are other forms that are supported on different ARM instruction sets but
generally the kernel just uses mcr as it is supported in all ARM instruction
sets.
Signed-off-by: Behan Webster
Reviewed-by: Mark Charlebois
Acked-by: Will Deacon
---
arch/arm/mach-vt8500/vt8500.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-vt8500/vt8500.c b/arch/arm/mach-vt8500/vt8500.c
index 2da7be3..3bc0dc9 100644
--- a/arch/arm/mach-vt8500/vt8500.c
+++ b/arch/arm/mach-vt8500/vt8500.c
@@ -69,7 +69,7 @@ static void vt8500_power_off(void)
{
local_irq_disable();
writew(5, pmc_base + VT8500_HCR_REG);
- asm("mcr%? p15, 0, %0, c7, c0, 4" : : "r" (0));
+ asm("mcr p15, 0, %0, c7, c0, 4" : : "r" (0));
}
static void __init vt8500_init(void)
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio, bcm-kona, LLVMLinux: Remove use of __initconst
On 09/23/14 14:29, Matt Porter wrote: On Tue, Sep 23, 2014 at 12:30:16PM -0700, beh...@converseincode.com wrote: From: Behan Webster The __initconst is in the wrong place, and when moved to the correct place it uncovers an error where the variable is used by non-init data structures. Instead merely make them const and put the const in the right spot. Signed-off-by: Behan Webster Reviewed-by: Mark Charlebois Acked-by: Arnd Bergmann --- drivers/gpio/gpio-bcm-kona.c | 2 +- drivers/mmc/host/sdhci-bcm-kona.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) As mentioned on IRC, Linus, Chris, and Ulf probably would like this split to go into each respective tree. Thanks Matt. I reposted as a 2 patch series. Strictly speaking one patch is for each tree. Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 01/12] crypto: LLVMLinux: Add macro to remove use of VLAIS in crypto code
On 09/17/14 04:30, Herbert Xu wrote: On Wed, Sep 17, 2014 at 02:15:40PM +0300, Dmitry Kasatkin wrote: On 17/09/14 12:22, Herbert Xu wrote: On Mon, Sep 15, 2014 at 12:30:23AM -0700, beh...@converseincode.com wrote: From: Behan Webster Add a macro which replaces the use of a Variable Length Array In Struct (VLAIS) with a C99 compliant equivalent. This macro instead allocates the appropriate amount of memory using an char array. The new code can be compiled with both gcc and clang. struct shash_desc contains a flexible array member member ctx declared with CRYPTO_MINALIGN_ATTR, so sizeof(struct shash_desc) aligns the beginning of the array declared after struct shash_desc with long long. No trailing padding is required because it is not a struct type that can be used in an array. The CRYPTO_MINALIGN_ATTR is required so that desc is aligned with long long as would be the case for a struct containing a member with CRYPTO_MINALIGN_ATTR. Signed-off-by: Behan Webster Acked-by: Herbert Xu Thanks, Just in case. I would still follow advice from "Michał Mirosław" to use shash##__desc[] Absolutely. I will be posting a v4 patchset . Just waiting a bit more for more comments on v3. The macro from v4 will look like this which I believe will satisfy the concern and indeed be safer than my previous version. +#define SHASH_DESC_ON_STACK(shash, tfm) \ + char __##shash##_desc[sizeof(struct shash_desc) +\ + crypto_shash_descsize(tfm)] CRYPTO_MINALIGN_ATTR; \ + struct shash_desc *shash = (struct shash_desc *)__##shash##_desc Hmm. Is it worth adding a comment with this macro explaining the reason this works? Essentially much of what is in the commit message? Oh yes of course. My ack is more about the approach. Wonderful! Indeed. I would have asked for you to wait for v4 anyways. :) Thank you, Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 11/12] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c
On 09/15/14 07:21, Linus Torvalds wrote: On Mon, Sep 15, 2014 at 12:30 AM, wrote: From: Behan Webster Replaced the use of a Variable Length Array In Struct (VLAIS) with a C99 compliant equivalent. This patch allocates the appropriate amount of memory using a char array using the SHASH_DESC_ON_STACK macro. You only made the first case use SHASH_DESC_ON_STACK, the two other cases you left in the ugly format. Was that just an oversight, or was there some reason for it? Oversight. Will Fix. Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: linux-next: manual merge of the llvmlinux tree with the tree
On 09/14/14 23:38, Stephen Rothwell wrote:
Hi all,
Today's linux-next merge of the llvmlinux tree got a conflict in
drivers/gpu/drm/msm/hdmi/hdmi.c between commit fc886107c556 ("drm/msm:
Change nested function to static function") from the watchdog tree and
commit 8f2c494adad0 ("msm, hdmi: LLVMLinux: Remove nested function from
HDMI driver") from the llvmlinux tree.
I fixed it up (these patches are doing the same thing, so I used the
version in the watchdog tree) and can carry the fix as necessary (no
action is required).
P.S. Behan, I noticed that the llvmlinux patch has no Signed-off-by
from you even though you committed it ...
Drat. That patch wasn't supposed to be in there. Sorry about that.
Yeah. That one was accepted upstream already. I was carrying it in my
repos so we could still build until it made it to mainline. I should not
have put that in linux-next. I actually just had to pull it out of my
own builds due to the same merge issue.
Hmm. Seems I put a couple of patches into the for-next branch late last
night which shouldn't have been. Fixed.
Apologies,
Behan
--
Behan Webster
beh...@converseincode.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH v3 01/12] crypto: LLVMLinux: Add macro to remove use of VLAIS in crypto code
On 09/15/14 01:06, Michał Mirosław wrote: 2014-09-15 9:30 GMT+02:00 : [...] +#define SHASH_DESC_ON_STACK(shash, tfm) \ + char __desc[sizeof(struct shash_desc) + \ + crypto_shash_descsize(tfm)] CRYPTO_MINALIGN_ATTR; \ + struct shash_desc *shash = (struct shash_desc *)__desc + char shash##__desc[] or similar? Otherwise it won't work if you use this macro twice in the same block. Best Regards, Michał Mirosław Good thinking. Will fix. Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] btrfs: LLVMLinux: Remove VLAIS
On 09/11/14 09:34, Chris Mason wrote:
On 09/05/2014 06:58 PM, beh...@converseincode.com wrote:
From: Vinícius Tinti
Replaced the use of a Variable Length Array In Struct (VLAIS) with a C99
compliant equivalent. This is the original VLAIS struct.
struct {
struct shash_desc shash;
char ctx[crypto_shash_descsize(tfm)];
} desc;
This patch instead allocates the appropriate amount of memory using an char
array.
The new code can be compiled with both gcc and clang.
struct shash_desc contains a flexible array member member ctx declared with
CRYPTO_MINALIGN_ATTR, so sizeof(struct shash_desc) aligns the beginning
of the array declared after struct shash_desc with long long.
No trailing padding is required because it is not a struct type that can
be used in an array.
The CRYPTO_MINALIGN_ATTR is required so that desc is aligned with long long
as would be the case for a struct containing a member with
CRYPTO_MINALIGN_ATTR.
We copied this from one of the other crypto api users, and I'm sure
cooking up all these patches was not a great way to your afternoon.
But, can I talk you into making a helper macro of some kind for this and
putting it into the crypto api headers? Honestly this setup seems
really error prone.
You're not the only person to ask for this. I've got one already coded
up based on suggestions from tglx.
I've regenerated all the patches. I will resubmit soon.
Will fix.
Behan
--
Behan Webster
beh...@converseincode.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] arm64: LLVMLinux: Provide __aeabi_* symbols which are needed for clang
On 09/08/14 16:01, Mark Charlebois wrote: On Sun, Sep 7, 2014 at 12:30 AM, Catalin Marinas wrote: On 7 Sep 2014, at 03:30, Mark Charlebois wrote: On Sat, Sep 6, 2014 at 7:16 AM, Arnd Bergmann wrote: On Friday 05 September 2014 16:23:14 beh...@converseincode.com wrote: + * Copyright (C) 2012 Mark Charlebois + */ + +/* + * EABI routines Does EABI specify these function names? I would think that they are just random libgcc (whatever that is called in clang) functions. http://infocenter.arm.com/help/topic/com.arm.doc.ihi0043d/IHI0043D_rtabi.pdf See section 4.3.4 Memory copying, clearing, and setting What does this document have to do with arm64 (AArch64, A64)? We don’t need such symbols on arm64. Also, the arm64 kernel links with libgcc (no immediate need AFAICT but the compiler does not guarantee the intrinsics would always be generated inline). [reposting in plain text] This patch was made early in the arm64 kernel support. I just retested and you are correct, it is no longer needed. My apologies to all. Whoops. I normally check whether a patch is still needed before posting them. I seem to have missed that step this time. My apologies as well. That all being said, I prefer to see patches no longer required, than needing them to be upstreamed, so in that this is a win. Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c
On 09/08/14 08:43, Mimi Zohar wrote: Behan, thank you for the explanation. No worries. I should have explained better. My apologies. The same snippet of code used here, and elsewhere in the kernel, is taken from the crypto subsystem. Once it is resolved in the crypto subsystem, the same solution should be propogated. Mimi Indeed that is my intention. I have tglx's suggested solution coded already. Just doing a bunch of allyesconfig builds to confirm all is compiling correctly. I will post all patches as a single patch set this time (posted to all concerned). I will repeat the explanation as well with the new patch set so everyone else in other subsystems sees those reasons as well. If this works for everyone I'll also go back and update the crypto patches for the subsystems that have already accepted my previous patches. Thanks, Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c
On 09/08/14 04:15, Dmitry Kasatkin wrote:
On 07/09/14 05:06, Behan Webster wrote:
On 09/06/14 03:11, Thomas Gleixner wrote:
On Fri, 5 Sep 2014, Behan Webster wrote:
On 09/05/14 17:18, Thomas Gleixner wrote:
Signed-off-by: Behan Webster
Signed-off-by: Mark Charlebois
Signed-off-by: Jan-Simon Möller
This SOB chain is completely ass backwards. See Documentation/...
"The Signed-off-by: tag indicates that the signer was involved in the
development of the patch, or that he/she was in the patch's delivery
path."
All three of us were involved. Does that not satisfy this rule?
No. Read #12
The sign-off is a simple line at the end of the explanation for the
patch, which certifies that you wrote it or otherwise have the right to
pass it on as an open-source patch.
So the above chain says:
Written-by: Behan
Passed-on-by: Mark
Passed-on-by: Jan
That would be correct if you sent the patch to Mark, Mark sent it to
Jan and Jan finally submitted it to LKML.
I suppose "Reviewed-by" is probably more appropriate for the last 2
then. Will fix.
-struct {
-struct shash_desc shash;
-char ctx[crypto_shash_descsize(tfm)];
-} desc;
+char desc[sizeof(struct shash_desc) +
+crypto_shash_descsize(tfm)] CRYPTO_MINALIGN_ATTR;
+struct shash_desc *shash = (struct shash_desc *)desc;
That anon struct should have never happened in the first place.
Sadly this is a design pattern used in many places through out the
kernel, and
appears to be fundamental to the crypto system. I was advised *not*
to change
it, so we haven't.
I agree that it's not a good practice.
Not
your problem, but you are not making it any better. You replace open
coded crap with even more unreadable crap.
Whats wrong with
SHASH_DESC_ON_STACK(shash, tfm);
Nothing is wrong with that. I would have actually preferred that.
But it would
have fundamentally changed a lot more code.
Errm. Why is
#define SHASH_DESC_ON_STACK(shash, tfm)\
char __shash[sizeof(.)];\
struct shash_desc *shash = (struct shash_desc *) __shash
requiring more fundamental than open coding the same thing a gazillion
times. You still need to change ALL usage sides of the anon struct.
So in fact you could avoid the whole code change by making it
SHASH_DESC_ON_STACK(desc, tfm);
and do the anon struct or a proper struct magic in the macro.
I see. I thought you meant a more fundamental change to the crypto
system API. My misunderstanding.
Ironically we tried to stay away from macros since the last time we
tried to replace VLAIS using macros (we've attempted patches to remove
VLAIS a few times) we were told *not* to hide the implementation with
macro magic. Though, to be fair, we were using more pointer math in
our other macro-based effort, and the non-crypto uses of VLAIS are a
lot more complex to replace.
Like I said I'm actually a fan of hiding ugliness in macros. Will fix.
Again, thanks for the feedback,
Behan
Hi,
Despite if it is crap or not, it was said already in this thread,
following "design pattern" is heavily used through out the kernel - by
crypto core itself and by many widely used clients.
struct {
struct shash_desc shash;
char ctx[crypto_shash_descsize(tfm)];
} desc;
My question why do you want to change this particular piece of code?
Because it employs Variable Length Arrays in Structs. A construct which
is explicitly forbidden by the C standard (C89, C99, C11). Because the
vast majority of kernel developers I've talked to about this have been
unaware of the use of VLAIS in the kernel and most find its use
objectionable (there is a similar objection to the use of nested
functions). Because implementing VLAIS in a compiler can severely impact
the generated instructions surrounding its use, which is why most
compilers don't implement VLAIS as a feature. Because using such a
construct precludes standards based compilers from competing with the
incumbent (my interest is enabling the use of clang and LLVM based
technologies as a toolchain choice to compile and develop the kernel).
What about rest of the kernel?
The LLVMLinux project is systematically working to remove the use of
VLAIS from the kernel (already removed from ext4, USB Gadget, netfilter,
mac802.11, apparmor, bluetooth, etc). Users of the crpyto subsystem are
one of the last and heaviest users of VLAIS.
To solve your problem you probably need to change everything.
Essentially yes. Though I like to think of it as finding alternatives to
where ever it is still used. "Changing everything" implies much larger
changes which aren't necessary in most cases. Sometimes the alternative
is merely using a flexible member (zero length array at the end of the
struct, instead of a VLA in the struct). In several places several VLAs
are used in the same struct. And rec
Re: [PATCH] arm64: LLVMLinux: Provide __aeabi_* symbols which are needed for clang
On 09/06/14 07:16, Arnd Bergmann wrote: On Friday 05 September 2014 16:23:14 beh...@converseincode.com wrote: --- /dev/null +++ b/arch/arm64/lib/eabi.c @@ -0,0 +1,32 @@ +/* + * linux/lib/eabi.c Please don't put the file names in the files themselves, it's redundant and in this case actually wrong. Will fix. + * Copyright (C) 2012 Mark Charlebois + */ + +/* + * EABI routines Does EABI specify these function names? I would think that they are just random libgcc (whatever that is called in clang) functions. These specialized functions are part of the ABI for the ARM architecture (AEABI). They aren't random. Memcpy and memmove *could* might be satisfied with linker magic instead. But memset uses the reverse parameter list. Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] crypto, dm: LLVMLinux: Remove VLAIS usage from dm-crypt
On 09/06/14 01:46, Milan Broz wrote:
On 09/06/2014 01:02 AM, beh...@converseincode.com wrote:
From: Jan-Simon Möller
The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
precludes the use of compilers which don't implement VLAIS (for instance the
Clang compiler). This patch instead allocates the appropriate amount of memory
using an char array.
Well, if clang (or C99 code) is now preferred for kernel, why not.
Just please commit the patch series en bloc (together with the patches
removing VLAIS from crypto code you posted to cryptoapi list).
They seemed more separate than that. However, happy to post them as a
patch set.
- struct {
- struct shash_desc desc;
- char ctx[crypto_shash_descsize(lmk->hash_tfm)];
- } sdesc;
+ char sdesc[sizeof(struct shash_desc) +
+ crypto_shash_descsize(lmk->hash_tfm)] CRYPTO_MINALIGN_ATTR;
+ struct shash_desc *desc = (struct shash_desc *)sdesc;
TBH, this looks even more uglier that the previous code :-)
I'm not claiming it's prettier. Merely C99. :)
(But tglx already complained on different patch and I fully agree that crypto
code
should not use this kind of construction in the first place...
It would be very nice to introduce at least some macro hiding these crazy
stack allocations later.)
I actually agree with you and tglx. Will fix.
As I said to tglx, we were asked not to hide things with macro magic in
some of our non-crypto VLAIS removal patches. Happy to use macros in
this case.
Thanks,
Behan
--
Behan Webster
beh...@converseincode.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c
On 09/06/14 03:11, Thomas Gleixner wrote:
On Fri, 5 Sep 2014, Behan Webster wrote:
On 09/05/14 17:18, Thomas Gleixner wrote:
Signed-off-by: Behan Webster
Signed-off-by: Mark Charlebois
Signed-off-by: Jan-Simon Möller
This SOB chain is completely ass backwards. See Documentation/...
"The Signed-off-by: tag indicates that the signer was involved in the
development of the patch, or that he/she was in the patch's delivery path."
All three of us were involved. Does that not satisfy this rule?
No. Read #12
The sign-off is a simple line at the end of the explanation for the
patch, which certifies that you wrote it or otherwise have the right to
pass it on as an open-source patch.
So the above chain says:
Written-by: Behan
Passed-on-by: Mark
Passed-on-by: Jan
That would be correct if you sent the patch to Mark, Mark sent it to
Jan and Jan finally submitted it to LKML.
I suppose "Reviewed-by" is probably more appropriate for the last 2
then. Will fix.
- struct {
- struct shash_desc shash;
- char ctx[crypto_shash_descsize(tfm)];
- } desc;
+ char desc[sizeof(struct shash_desc) +
+ crypto_shash_descsize(tfm)] CRYPTO_MINALIGN_ATTR;
+ struct shash_desc *shash = (struct shash_desc *)desc;
That anon struct should have never happened in the first place.
Sadly this is a design pattern used in many places through out the kernel, and
appears to be fundamental to the crypto system. I was advised *not* to change
it, so we haven't.
I agree that it's not a good practice.
Not
your problem, but you are not making it any better. You replace open
coded crap with even more unreadable crap.
Whats wrong with
SHASH_DESC_ON_STACK(shash, tfm);
Nothing is wrong with that. I would have actually preferred that. But it would
have fundamentally changed a lot more code.
Errm. Why is
#define SHASH_DESC_ON_STACK(shash, tfm) \
char __shash[sizeof(.)];\
struct shash_desc *shash = (struct shash_desc *) __shash
requiring more fundamental than open coding the same thing a gazillion
times. You still need to change ALL usage sides of the anon struct.
So in fact you could avoid the whole code change by making it
SHASH_DESC_ON_STACK(desc, tfm);
and do the anon struct or a proper struct magic in the macro.
I see. I thought you meant a more fundamental change to the crypto
system API. My misunderstanding.
Ironically we tried to stay away from macros since the last time we
tried to replace VLAIS using macros (we've attempted patches to remove
VLAIS a few times) we were told *not* to hide the implementation with
macro magic. Though, to be fair, we were using more pointer math in our
other macro-based effort, and the non-crypto uses of VLAIS are a lot
more complex to replace.
Like I said I'm actually a fan of hiding ugliness in macros. Will fix.
Again, thanks for the feedback,
Behan
--
Behan Webster
beh...@converseincode.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c
On 09/05/14 17:18, Thomas Gleixner wrote:
On Fri, 5 Sep 2014, beh...@converseincode.com wrote:
From: Behan Webster
Replaced the use of a Variable Length Array In Struct (VLAIS) with a C99
compliant equivalent. This patch allocates the appropriate amount of memory
using an char array.
The new code can be compiled with both gcc and clang.
struct shash_desc contains a flexible array member member ctx declared with
CRYPTO_MINALIGN_ATTR, so sizeof(struct shash_desc) aligns the beginning
of the array declared after struct shash_desc with long long.
No trailing padding is required because it is not a struct type that can
be used in an array.
The CRYPTO_MINALIGN_ATTR is required so that desc is aligned with long long
as would be the case for a struct containing a member with
CRYPTO_MINALIGN_ATTR.
Signed-off-by: Behan Webster
Signed-off-by: Mark Charlebois
Signed-off-by: Jan-Simon Möller
This SOB chain is completely ass backwards. See Documentation/...
"The Signed-off-by: tag indicates that the signer was involved in the
development of the patch, or that he/she was in the patch's delivery path."
All three of us were involved. Does that not satisfy this rule?
---
security/integrity/ima/ima_crypto.c | 53 +
1 file changed, 25 insertions(+), 28 deletions(-)
diff --git a/security/integrity/ima/ima_crypto.c
b/security/integrity/ima/ima_crypto.c
index 0bd7328..a6aa2b0 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -380,17 +380,16 @@ static int ima_calc_file_hash_tfm(struct file *file,
loff_t i_size, offset = 0;
char *rbuf;
int rc, read = 0;
- struct {
- struct shash_desc shash;
- char ctx[crypto_shash_descsize(tfm)];
- } desc;
+ char desc[sizeof(struct shash_desc) +
+ crypto_shash_descsize(tfm)] CRYPTO_MINALIGN_ATTR;
+ struct shash_desc *shash = (struct shash_desc *)desc;
That anon struct should have never happened in the first place.
Sadly this is a design pattern used in many places through out the
kernel, and appears to be fundamental to the crypto system. I was
advised *not* to change it, so we haven't.
I agree that it's not a good practice.
Not
your problem, but you are not making it any better. You replace open
coded crap with even more unreadable crap.
Whats wrong with
SHASH_DESC_ON_STACK(shash, tfm);
Nothing is wrong with that. I would have actually preferred that. But it
would have fundamentally changed a lot more code.
I'm not sure making fundamental changes to the crypto code in order to
make "my favourite compiler happy" is really a better plan. I prefer
smaller more measured steps.
or something along that line and hide all the stack allocation, type
conversion crap and make my favourite compiler happy in a single
place?
At this point it is less about making a particular compiler happy, and
more about making the code at least C99 compliant which enables a
different compiler so that more people can use it to make more
fundamental changes like you are suggesting.
I appreciate your feedback,
Behan
--
Behan Webster
beh...@converseincode.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 5/6] apparmor: LLVMLinux: Remove VLAIS
On 09/02/14 16:16, John Johansen wrote: I'm fine with this, do you want me to pull it into my tree for our next push or do you want this all to go together as a set? Acked-by: John Johansen I'm more than happy for individual maintainers to pull relevant patches into their trees for the next merge window. They were presented as a RFC patch set purely to get consensus as a group since its the same change across the board. Happy to push your patch to you now. Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 0/6] LLVMLinux: Patches to enable the kernel to be compiled with clang/LLVM
On 09/02/14 16:01, Marcel Holtmann wrote: Hi Behan, These patches remove the use of Variable Length Arrays In Structs (VLAIS) in crypto related code. Presented here for comments as a whole (since they all do the same thing in the same way). Once everyone is happy I will submit them individually to their appropriate maintainers. The LLVMLinux project aims to fully build the Linux kernel using both gcc and clang (the C front end for the LLVM compiler infrastructure project). Jan-Simon Möller (4): crypto, dm: LLVMLinux: Remove VLAIS usage from dm-crypt crypto: LLVMLinux: Remove VLAIS usage from crypto/hmac.c crypto: LLVMLinux: Remove VLAIS usage from libcrc32c.c crypto: LLVMLinux: Remove VLAIS usage from crypto/testmgr.c Vinícius Tinti (2): apparmor: LLVMLinux: Remove VLAIS btrfs: LLVMLinux: Remove VLAIS crypto/hmac.c | 27 +-- crypto/testmgr.c | 16 drivers/md/dm-crypt.c | 38 ++ fs/btrfs/hash.c| 18 +- lib/libcrc32c.c| 18 +- security/apparmor/crypto.c | 19 +-- 6 files changed, 66 insertions(+), 70 deletions(-) are you sure these are all of them? I know for a fact that we are using the same construct in net/bluetooth/amp.c as well. There have been other places where this was an issue before too (ext4, mac80211, USB gadget, ...). Some have already been fixed. Hmm. Yeah, I thought we had a patch for bluetooth too. I can't find it now though. Suffice it to say that similar patches are required for the other instances of this kind of code elsewhere as well. Thanks Marcel, Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/4] LLVMLinux: Patches to enable the kernel to be compiled with clang/LLVM
On 08/26/14 07:16, Will Deacon wrote: Hi Behan, On Fri, Aug 01, 2014 at 05:11:59AM +0100, Behan Webster wrote: On 07/31/14 03:33, Will Deacon wrote: On Thu, Jul 31, 2014 at 12:57:25AM +0100, beh...@converseincode.com wrote: From: Behan Webster This patch set moves from using locally defined named registers to access the stack pointer to using a globally defined named register. This allows the code to work both with gcc and clang. The LLVMLinux project aims to fully build the Linux kernel using both gcc and clang (the C front end for the LLVM compiler infrastructure project). Behan Webster (4): arm64: LLVMLinux: Add current_stack_pointer() for arm64 arm64: LLVMLinux: Use current_stack_pointer in save_stack_trace_tsk arm64: LLVMLinux: Calculate current_thread_info from current_stack_pointer arm64: LLVMLinux: Use current_stack_pointer in kernel/traps.c Once Andreas's comments have been addressed: Acked-by: Will Deacon Please can you send a new series after the merge window? Pity. I was hoping to get it in this merge window. However, will resubmit for 3.18. Any chance of a v2 for this series, please? If you address the comments pending for v1, I think it's good to merge. Sure thing. 2 more named register patches added. Look for them at the end of the new patch series. I kept missing you in Chicago. I was hoping to say "hi". Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/4] LLVMLinux: Patches to enable the kernel to be compiled with clang/LLVM
On 07/31/14 03:33, Will Deacon wrote: On Thu, Jul 31, 2014 at 12:57:25AM +0100, beh...@converseincode.com wrote: From: Behan Webster This patch set moves from using locally defined named registers to access the stack pointer to using a globally defined named register. This allows the code to work both with gcc and clang. The LLVMLinux project aims to fully build the Linux kernel using both gcc and clang (the C front end for the LLVM compiler infrastructure project). Behan Webster (4): arm64: LLVMLinux: Add current_stack_pointer() for arm64 arm64: LLVMLinux: Use current_stack_pointer in save_stack_trace_tsk arm64: LLVMLinux: Calculate current_thread_info from current_stack_pointer arm64: LLVMLinux: Use current_stack_pointer in kernel/traps.c Once Andreas's comments have been addressed: Acked-by: Will Deacon Please can you send a new series after the merge window? Pity. I was hoping to get it in this merge window. However, will resubmit for 3.18. Thanks, Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] kbuild, LLVMLinux: Supress warnings unless W=1-3
On 07/31/14 13:46, Michal Marek wrote: Dne 31.7.2014 18:12, Behan Webster napsal(a): On 07/31/14 01:18, Michal Marek wrote: Dne 31.7.2014 06:16, beh...@converseincode.com napsal(a): @@ -55,6 +45,18 @@ warning-3 += -Wswitch-default warning-3 += $(call cc-option, -Wpacked-bitfield-compat) warning-3 += $(call cc-option, -Wvla) +ifeq ($(COMPILER),clang) +ifndef $(W) +KBUILD_CFLAGS += $(call cc-disable-warning, initializer-overrides) +KBUILD_CFLAGS += $(call cc-disable-warning, unused-value) +KBUILD_CFLAGS += $(call cc-disable-warning, format) +KBUILD_CFLAGS += $(call cc-disable-warning, unknown-warning-option) +KBUILD_CFLAGS += $(call cc-disable-warning, sign-compare) +KBUILD_CFLAGS += $(call cc-disable-warning, format-zero-length) +KBUILD_CFLAGS += $(call cc-disable-warning, uninitialized) +endif +endif + Please remove this part, it has no effect. I assume that if it works for you, these warning are not as annoying so they do not need to be disabled? Actually they are annoying, that's why they're disabled normally. Most of them complain about practices which are relatively common in kernel code. clang warns about a lot more things than gcc does. It means that code which compiles cleanly in gcc often doesn't with clang. This cuts out the warnings which are unlikely to to be fixed in kernel code anytime soon, but which are probably worth exposing when W=1 is used. This part of the patch explicitly deals with complaints from some in the kernel community that clang is too noisy with kernel code. This part of the patch needs to be somewhere. This seemed the best place. You placed it inside a branch that is only evaluated when W= is given. Hmm. You're right. Will fix. Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] kbuild, LLVMLinux: Supress warnings unless W=1-3
On 07/31/14 01:18, Michal Marek wrote: Dne 31.7.2014 06:16, beh...@converseincode.com napsal(a): From: Behan Webster clang has more warnings enabled by default. Turn them off unless W is set. This patch fixes a logic bug where warnings in clang were disabled when W was set. Signed-off-by: Behan Webster Signed-off-by: Jan-Simon Möller Signed-off-by: Mark Charlebois Cc: mma...@suse.cz Cc: b...@alien8.de --- Makefile | 1 + scripts/Makefile.extrawarn | 22 -- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/Makefile b/Makefile index f6a7794..f343e17 100644 --- a/Makefile +++ b/Makefile @@ -668,6 +668,7 @@ KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare) # source of a reference will be _MergedGlobals and not on of the whitelisted names. # See modpost pattern 2 KBUILD_CFLAGS += $(call cc-option, -mno-global-merge,) +KBUILD_CFLAGS += $(call cc-option, -fcatch-undefined-behavior) else # This warning generated too much noise in a regular build. diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn index 6564350..b5b0751 100644 --- a/scripts/Makefile.extrawarn +++ b/scripts/Makefile.extrawarn @@ -26,16 +26,6 @@ warning-1 += $(call cc-option, -Wmissing-include-dirs) warning-1 += $(call cc-option, -Wunused-but-set-variable) warning-1 += $(call cc-disable-warning, missing-field-initializers) -# Clang -warning-1 += $(call cc-disable-warning, initializer-overrides) -warning-1 += $(call cc-disable-warning, unused-value) -warning-1 += $(call cc-disable-warning, format) -warning-1 += $(call cc-disable-warning, unknown-warning-option) -warning-1 += $(call cc-disable-warning, sign-compare) -warning-1 += $(call cc-disable-warning, format-zero-length) -warning-1 += $(call cc-disable-warning, uninitialized) -warning-1 += $(call cc-option, -fcatch-undefined-behavior) - OK. warning-2 := -Waggregate-return warning-2 += -Wcast-align warning-2 += -Wdisabled-optimization @@ -55,6 +45,18 @@ warning-3 += -Wswitch-default warning-3 += $(call cc-option, -Wpacked-bitfield-compat) warning-3 += $(call cc-option, -Wvla) +ifeq ($(COMPILER),clang) +ifndef $(W) +KBUILD_CFLAGS += $(call cc-disable-warning, initializer-overrides) +KBUILD_CFLAGS += $(call cc-disable-warning, unused-value) +KBUILD_CFLAGS += $(call cc-disable-warning, format) +KBUILD_CFLAGS += $(call cc-disable-warning, unknown-warning-option) +KBUILD_CFLAGS += $(call cc-disable-warning, sign-compare) +KBUILD_CFLAGS += $(call cc-disable-warning, format-zero-length) +KBUILD_CFLAGS += $(call cc-disable-warning, uninitialized) +endif +endif + Please remove this part, it has no effect. I assume that if it works for you, these warning are not as annoying so they do not need to be disabled? Actually they are annoying, that's why they're disabled normally. Most of them complain about practices which are relatively common in kernel code. clang warns about a lot more things than gcc does. It means that code which compiles cleanly in gcc often doesn't with clang. This cuts out the warnings which are unlikely to to be fixed in kernel code anytime soon, but which are probably worth exposing when W=1 is used. This part of the patch explicitly deals with complaints from some in the kernel community that clang is too noisy with kernel code. This part of the patch needs to be somewhere. This seemed the best place. Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] arm64: LLVMLinux: Calculate current_thread_info from current_stack_pointer
On 07/30/14 22:31, Andreas Färber wrote:
Hi,
Am 31.07.2014 01:57, schrieb beh...@converseincode.com:
From: Behan Webster
Use the global current_stack_pointer to get the value of the stack pointer.
This change supports being able to compile the kernel with both gcc and clang.
Signed-off-by: Behan Webster
Signed-off-by: Mark Charlebois
Reviewed-by: Jan-Simon M??ller
Something went wrong with ö encoding here and in 2/4.
Yeah. That keeps happening. :(
Thanks.
---
arch/arm64/include/asm/thread_info.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/include/asm/thread_info.h
b/arch/arm64/include/asm/thread_info.h
index e6b6094..c2432d2 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -80,8 +80,8 @@ static inline struct thread_info *current_thread_info(void)
__attribute_const__;
static inline struct thread_info *current_thread_info(void)
{
- register unsigned long sp asm ("sp");
- return (struct thread_info *)(sp & ~(THREAD_SIZE - 1));
+ return (struct thread_info *) \
This is not a macro, so \ seems superfluous.
Doh. Indeed. Will fix.
Behan
--
Behan Webster
beh...@converseincode.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kbuild, LLVMLinux: only use warnings when using clang
On 07/30/14 06:04, Michal Marek wrote: On 2014-07-01 12:12, Borislav Petkov wrote: On Mon, Jun 30, 2014 at 05:42:26PM -0700, beh...@converseincode.com wrote: From: Behan Webster Only consider clang warnings in Kbuild when using the clang compiler. Signed-off-by: Behan Webster --- scripts/Makefile.extrawarn | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn index 6564350..e350127 100644 --- a/scripts/Makefile.extrawarn +++ b/scripts/Makefile.extrawarn @@ -26,7 +26,7 @@ warning-1 += $(call cc-option, -Wmissing-include-dirs) warning-1 += $(call cc-option, -Wunused-but-set-variable) warning-1 += $(call cc-disable-warning, missing-field-initializers) -# Clang +ifeq ($(COMPILER),clang) warning-1 += $(call cc-disable-warning, initializer-overrides) warning-1 += $(call cc-disable-warning, unused-value) warning-1 += $(call cc-disable-warning, format) @@ -35,6 +35,7 @@ warning-1 += $(call cc-disable-warning, sign-compare) warning-1 += $(call cc-disable-warning, format-zero-length) warning-1 += $(call cc-disable-warning, uninitialized) warning-1 += $(call cc-option, -fcatch-undefined-behavior) +endif Ok, just to make sure I understand that whole use case correctly: The disabling of those warnings is really intended for the case where people build the kernel with "W=1" on the make cmdline *and* clang? Behan, Jan-Simon, can you explain why are those -Wno-... options needed in the W=1 case? The whole point of the W= option is to enable noisy warnings, so I don't quite get why you want to silence these. Sorry for the delay. That code is indeed not what was intended. It's more that these warnings should normally be disabled, and when W is set they should not be disabled. Just putting the final touches on a patch which addresses this situation. Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/6] LLVMLinux: Patches to enable the kernel to be compiled with clang/LLVM
On 07/13/14 02:10, Nicolas Pitre wrote: On Tue, 8 Jul 2014, beh...@converseincode.com wrote: From: Behan Webster The LLVMLinux project aims to fully build the Linux kernel using both gcc and clang (the C front end for the LLVM compiler infrastructure project). Clang only supports global named registers for non-allocatable registers like the stack pointer. By centralizing the definition of current_stack_pointer, the use of named registers for ARM remains largely unchanged while working for both gcc and clang. You verified that the compiled code is identical on gcc? Yes. Identical. If so: Acked-by: Nicolas Pitre Thanks, Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[GIT PULL] LLVMLinux patches for v3.16
Linus, Next set of patches to support compiling the kernel with clang. They've been soaking in linux-next since the last merge window. More still in the works for the next merge window... Thanks, Behan -- Behan Webster beh...@converseincode.com The following changes since commit d4c54919ed86302094c0ca7d48a8cbd4ee753e92: mm: add !pte_present() check on existing hugetlb_entry callbacks (2014-06-06 13:21:16 -0700) are available in the git repository at: git://git.linuxfoundation.org/llvmlinux/kernel.git tags/llvmlinux-for-v3.16 for you to fetch changes up to e6b80757700a11315dd81b161f8d86099214c185: arm, unwind, LLVMLinux: Enable clang to be used for unwinding the stack (2014-06-07 11:44:39 -0700) LLVMLinux patches for v3.16 Behan Webster (2): all: LLVMLinux: Change DWARF flag to support gcc and clang ARM: LLVMLinux: Change "extern inline" to "static inline" in glue-cache.h Mark Charlebois (3): crypto: LLVMLinux: aligned-attribute.patch net: netfilter: LLVMLinux: vlais-netfilter arm, unwind, LLVMLinux: Enable clang to be used for unwinding the stack Makefile | 2 +- arch/arm/include/asm/glue-cache.h | 22 +++--- arch/arm/kernel/unwind.c | 2 +- crypto/shash.c| 3 ++- net/netfilter/xt_repldata.h | 22 +- 5 files changed, 32 insertions(+), 19 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mpilib: use 'static inline' for mpi-inline.h
On 05/08/14 08:30, Steven Rostedt wrote: On Thu, 8 May 2014 16:56:20 +0200 Arnd Bergmann wrote: Four functions of mpilib are defined as 'extern inline' at the moment, which is generally not the correct way to do inline functions using gcc. I think clang would appreciate this too. Indeed. Saves me writing the same patch. :) Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[tip:x86/urgent] x86: LLVMLinux: Wrap -mno-80387 with cc-option
Commit-ID: 8f2dd677bec68fb55904799a82674c9e64b23be3 Gitweb: http://git.kernel.org/tip/8f2dd677bec68fb55904799a82674c9e64b23be3 Author: Behan Webster AuthorDate: Mon, 21 Apr 2014 22:40:27 -0700 Committer: Ingo Molnar CommitDate: Tue, 22 Apr 2014 11:41:16 +0200 x86: LLVMLinux: Wrap -mno-80387 with cc-option Wrap -mno-80387 gcc options with cc-option so they don't break clang. Signed-off-by: Behan Webster Cc: torva...@linux-foundation.org Cc: dw...@infradead.org Cc: pagee...@freemail.hu Link: http://lkml.kernel.org/r/1398145227-25053-1-git-send-email-beh...@converseincode.com Signed-off-by: Ingo Molnar --- arch/x86/Makefile | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/arch/x86/Makefile b/arch/x86/Makefile index d1b7c37..ce6ad7e 100644 --- a/arch/x86/Makefile +++ b/arch/x86/Makefile @@ -83,7 +83,9 @@ else KBUILD_CFLAGS += -m64 # Don't autogenerate traditional x87, MMX or SSE instructions -KBUILD_CFLAGS += -mno-mmx -mno-sse -mno-80387 -mno-fp-ret-in-387 +KBUILD_CFLAGS += -mno-mmx -mno-sse +KBUILD_CFLAGS += $(call cc-option,-mno-80387) +KBUILD_CFLAGS += $(call cc-option,-mno-fp-ret-in-387) # Use -mpreferred-stack-boundary=3 if supported. KBUILD_CFLAGS += $(call cc-option,-mpreferred-stack-boundary=3) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86: LLVMLinux: Wrap -mno-80387 with cc-option
On 04/21/14 23:05, Ingo Molnar wrote: * beh...@converseincode.com wrote: From: Behan Webster Wrap -mno-80387 gcc options with cc-option so they don't break clang. Signed-off-by: Behan Webster --- arch/x86/Makefile | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/arch/x86/Makefile b/arch/x86/Makefile index d1b7c37..ce6ad7e 100644 --- a/arch/x86/Makefile +++ b/arch/x86/Makefile @@ -83,7 +83,9 @@ else KBUILD_CFLAGS += -m64 # Don't autogenerate traditional x87, MMX or SSE instructions -KBUILD_CFLAGS += -mno-mmx -mno-sse -mno-80387 -mno-fp-ret-in-387 +KBUILD_CFLAGS += -mno-mmx -mno-sse +KBUILD_CFLAGS += $(call cc-option,-mno-80387) +KBUILD_CFLAGS += $(call cc-option,-mno-fp-ret-in-387) Is there a clang equivalent option that inhibits all things FPU opcode generation by the compiler? Not that I've found yet. Still investigating. That's the general purpose of -no-80387. Yes, I understand what this is trying to accomplish. At this point I'd just like this new code not to break the use of clang with v3.15. I will submit another patch which adds similar functionality when compiled with clang once I have that answer. Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4] net: netfilter: LLVMLinux: vlais-netfilter
On 03/22/14 23:55, beh...@converseincode.com wrote:
From: Mark Charlebois
Replaced non-standard C use of Variable Length Arrays In Structs (VLAIS) in
xt_repldata.h with a C99 compliant flexible array member and then calculated
offsets to the other struct members. These other members aren't referenced by
name in this code, however this patch maintains the same memory layout and
padding as was previously accomplished using VLAIS.
Had the original structure been ordered differently, with the entries VLA at
the end, then it could have been a flexible member, and this patch would have
been a lot simpler. However since the data stored in this structure is
ultimately exported to userspace, the order of this structure can't be changed.
This patch makes no attempt to change the existing behavior, merely the way in
which the current layout is accomplished using standard C99 constructs. As such
the code can now be compiled with either gcc or clang.
This version of the patch removes the trailing alignment that the VLAIS
structure would allocate in order to simplify the patch.
Author: Mark Charlebois
Signed-off-by: Mark Charlebois
Signed-off-by: Behan Webster
Signed-off-by: Vinícius Tinti
---
net/netfilter/xt_repldata.h | 22 +-
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/net/netfilter/xt_repldata.h b/net/netfilter/xt_repldata.h
index 6efe4e5..8fd3241 100644
--- a/net/netfilter/xt_repldata.h
+++ b/net/netfilter/xt_repldata.h
@@ -5,23 +5,35 @@
* they serve as the hanging-off data accessed through repl.data[].
*/
+/* tbl has the following structure equivalent, but is C99 compliant:
+ * struct {
+ * struct type##_replace repl;
+ * struct type##_standard entries[nhooks];
+ * struct type##_error term;
+ * } *tbl;
+ */
+
#define xt_alloc_initial_table(type, typ2) ({ \
unsigned int hook_mask = info->valid_hooks; \
unsigned int nhooks = hweight32(hook_mask); \
unsigned int bytes = 0, hooknum = 0, i = 0; \
struct { \
struct type##_replace repl; \
- struct type##_standard entries[nhooks]; \
- struct type##_error term; \
- } *tbl = kzalloc(sizeof(*tbl), GFP_KERNEL); \
+ struct type##_standard entries[]; \
+ } *tbl; \
+ struct type##_error *term; \
+ size_t term_offset = (offsetof(typeof(*tbl), entries[nhooks]) + \
+ __alignof__(*term) - 1) & ~(__alignof__(*term) - 1); \
+ tbl = kzalloc(term_offset + sizeof(*term), GFP_KERNEL); \
if (tbl == NULL) \
return NULL; \
+ term = (struct type##_error *)&(((char *)tbl)[term_offset]); \
strncpy(tbl->repl.name, info->name, sizeof(tbl->repl.name)); \
- tbl->term = (struct type##_error)typ2##_ERROR_INIT; \
+ *term = (struct type##_error)typ2##_ERROR_INIT; \
tbl->repl.valid_hooks = hook_mask; \
tbl->repl.num_entries = nhooks + 1; \
tbl->repl.size = nhooks * sizeof(struct type##_standard) + \
-sizeof(struct type##_error); \
+sizeof(struct type##_error); \
for (; hook_mask != 0; hook_mask >>= 1, ++hooknum) { \
if (!(hook_mask & 1)) \
continue; \
Any further feedback about this patch?
Behan
--
Behan Webster
beh...@converseincode.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[GIT PULL] LLVMLinux patches for v3.15
Linus, These are some initial updates to support compiling the kernel with clang. These patches have been through the proper reviews to the best of my ability, and have been soaking in linux-next for a few weeks. These patches by themselves still do not completely allow clang to be used with the kernel code, but lay the foundation for other patches which are still under review. Several other of the LLVMLinux patches have been already added via Maintainer trees. Thanks, Behan -- Behan Webster beh...@converseincode.com The following changes since commit 39de65aa2c3eee901db020a4f1396998e09602a3: Merge branch 'i2c/for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux (2014-04-09 08:45:40 -0700) are available in the git repository at: git://git.linuxfoundation.org/llvmlinux/kernel.git tags/llvmlinux-for-v3.15 for you to fetch changes up to c4586256f0c440bc2bdb29d2cbb915f0ca785d26: x86: LLVMLinux: Fix "incomplete type const struct x86cpu_device_id" (2014-04-09 13:44:35 -0700) LLVMLinux Patches for v3.15 -------- Behan Webster (4): kbuild: LLVMLinux: Add Kbuild support for building kernel with Clang LLVMLinux: Remove warning about returning an uninitialized variable x86, acpi: LLVMLinux: Remove nested functions from Thinkpad ACPI x86: LLVMLinux: Fix "incomplete type const struct x86cpu_device_id" Jan-Simon Möller (4): kbuild: LLVMLinux: Adapt warnings for compilation with clang Documentation: LLVMLinux: Update Documentation/dontdiff kbuild: LLVMLinux: Fix LINUX_COMPILER definition script for compilation with clang x86 kbuild: LLVMLinux: More cc-options added for clang Mark Charlebois (1): LLVMLinux: Add support for clang to compiler.h and new compiler-clang.h Documentation/dontdiff | 3 ++ Makefile | 30 - arch/x86/Makefile| 2 +- drivers/platform/x86/thinkpad_acpi.c | 86 +++- include/asm-generic/cmpxchg-local.h | 3 +- include/linux/compiler-clang.h | 12 + include/linux/compiler.h | 7 +++ include/linux/mod_devicetable.h | 5 +++ scripts/Makefile.build | 12 - scripts/mkcompile_h | 2 +- 10 files changed, 116 insertions(+), 46 deletions(-) create mode 100644 include/linux/compiler-clang.h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] LLVMLinux: Remove warning about returning an uninitialized variable
On 03/31/14 15:11, H. Peter Anvin wrote: On 03/31/2014 03:10 PM, Behan Webster wrote: diff --git a/include/asm-generic/cmpxchg-local.h b/include/asm-generic/cmpxchg-local.h index d8d4c89..4c41bb8 100644 --- a/include/asm-generic/cmpxchg-local.h +++ b/include/asm-generic/cmpxchg-local.h @@ -41,6 +41,7 @@ static inline unsigned long __cmpxchg_local_generic(volatile void *ptr, break; default: wrong_size_cmpxchg(ptr); +prev = 0; } raw_local_irq_restore(flags); return prev; Shouldn't this be a build time assert (__compiletime_error())? I changed it to a __noreturn on wrong_size_cmpxchg thanks to James Bottomley. Which would be better? __compiletime_error traps at compile time rather than link time, which is what you want. The idea is to remove the compile time warning that the return code "prev" isn't initialized in the default case. Indicating that wrong_size_cmpxchg doesn't return fixes that false positive. Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] LLVMLinux: Remove warning about returning an uninitialized variable
On 03/31/14 13:52, H. Peter Anvin wrote: On 03/21/2014 11:38 PM, beh...@converseincode.com wrote: From: Behan Webster Fix uninitialized return code in default case in cmpxchg-local.h This patch fixes the code to prevent an uninitialized return value that is detected when compiling with clang. The bug produces numerous warnings when compiling the Linux kernel with clang. Signed-off-by: Behan Webster Signed-off-by: Mark Charlebois --- include/asm-generic/cmpxchg-local.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/asm-generic/cmpxchg-local.h b/include/asm-generic/cmpxchg-local.h index d8d4c89..4c41bb8 100644 --- a/include/asm-generic/cmpxchg-local.h +++ b/include/asm-generic/cmpxchg-local.h @@ -41,6 +41,7 @@ static inline unsigned long __cmpxchg_local_generic(volatile void *ptr, break; default: wrong_size_cmpxchg(ptr); + prev = 0; } raw_local_irq_restore(flags); return prev; Shouldn't this be a build time assert (__compiletime_error())? I changed it to a __noreturn on wrong_size_cmpxchg thanks to James Bottomley. Which would be better? Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] LLVMLinux: Remove warning about returning an uninitialized variable
On 03/22/14 09:42, James Bottomley wrote: On Sat, 2014-03-22 at 09:37 -0700, Behan Webster wrote: On 03/22/14 09:29, James Bottomley wrote: On Sat, 2014-03-22 at 08:48 -0700, beh...@converseincode.com wrote: From: Behan Webster Fix uninitialized return code in default case in cmpxchg-local.h This patch fixes the code to prevent an uninitialized return value that is detected when compiling with clang. The bug produces numerous warnings when compiling the Linux kernel with clang. Signed-off-by: Behan Webster Signed-off-by: Mark Charlebois --- include/asm-generic/cmpxchg-local.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/asm-generic/cmpxchg-local.h b/include/asm-generic/cmpxchg-local.h index d8d4c89..9112111 100644 --- a/include/asm-generic/cmpxchg-local.h +++ b/include/asm-generic/cmpxchg-local.h @@ -41,6 +41,7 @@ static inline unsigned long __cmpxchg_local_generic(volatile void *ptr, break; default: wrong_size_cmpxchg(ptr); + __builtin_unreachable(); No, that's got to be unreachable() so that it works in all compilers, (__builtin_unreachable is a gcc-4 ism). It is also supported by clang. OK, but it's not supported by gcc-3. So gcc-3 would crap out seeing the statement, which is why we have to wrapper it. Sorry. I wasn't clear. It is not merely a gcc-4 ism; it is also supported by clang. I'm not arguing it should be used. I understand (and agree with) your objection to its use since it isn't supported before gcc-4. Got to say this still looks wrong. If wrong_size_cmpxchg() cannot return, the function should be annotated as such with __noreturn (like panic()) so the unreachable() should be superfluous. Okay. I can try that instead. Great; This seems to work for me (but then my compiler doesn't see the unreachable problem). James --- diff --git a/include/asm-generic/cmpxchg-local.h b/include/asm-generic/cmpxchg-local.h index d8d4c89..70bef78 100644 --- a/include/asm-generic/cmpxchg-local.h +++ b/include/asm-generic/cmpxchg-local.h @@ -4,7 +4,8 @@ #include #include -extern unsigned long wrong_size_cmpxchg(volatile void *ptr); +extern unsigned long wrong_size_cmpxchg(volatile void *ptr) + __noreturn; /* * Generic version of __cmpxchg_local (disables interrupts). Takes an unsigned Nice. I'll give it a try. It looks like a much better solution. Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] LLVMLinux: Remove warning about returning an uninitialized variable
On 03/22/14 09:29, James Bottomley wrote: On Sat, 2014-03-22 at 08:48 -0700, beh...@converseincode.com wrote: From: Behan Webster Fix uninitialized return code in default case in cmpxchg-local.h This patch fixes the code to prevent an uninitialized return value that is detected when compiling with clang. The bug produces numerous warnings when compiling the Linux kernel with clang. Signed-off-by: Behan Webster Signed-off-by: Mark Charlebois --- include/asm-generic/cmpxchg-local.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/asm-generic/cmpxchg-local.h b/include/asm-generic/cmpxchg-local.h index d8d4c89..9112111 100644 --- a/include/asm-generic/cmpxchg-local.h +++ b/include/asm-generic/cmpxchg-local.h @@ -41,6 +41,7 @@ static inline unsigned long __cmpxchg_local_generic(volatile void *ptr, break; default: wrong_size_cmpxchg(ptr); + __builtin_unreachable(); No, that's got to be unreachable() so that it works in all compilers, (__builtin_unreachable is a gcc-4 ism). It is also supported by clang. Got to say this still looks wrong. If wrong_size_cmpxchg() cannot return, the function should be annotated as such with __noreturn (like panic()) so the unreachable() should be superfluous. Okay. I can try that instead. Thanks, Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] LLVMLinux: Remove warning about returning an uninitialized variable
On 03/22/14 09:21, Sam Ravnborg wrote: On Sat, Mar 22, 2014 at 08:48:19AM -0700, beh...@converseincode.com wrote: From: Behan Webster Fix uninitialized return code in default case in cmpxchg-local.h This patch fixes the code to prevent an uninitialized return value that is detected when compiling with clang. The bug produces numerous warnings when compiling the Linux kernel with clang. Signed-off-by: Behan Webster Signed-off-by: Mark Charlebois --- include/asm-generic/cmpxchg-local.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/asm-generic/cmpxchg-local.h b/include/asm-generic/cmpxchg-local.h index d8d4c89..9112111 100644 --- a/include/asm-generic/cmpxchg-local.h +++ b/include/asm-generic/cmpxchg-local.h @@ -41,6 +41,7 @@ static inline unsigned long __cmpxchg_local_generic(volatile void *ptr, break; default: wrong_size_cmpxchg(ptr); + __builtin_unreachable(); It is unreachable() - see compiler.h Oh I see. Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] LLVMLinux: Remove warning about returning an uninitialized variable
On 03/22/14 03:01, Arnd Bergmann wrote: On Saturday 22 March 2014, beh...@converseincode.com wrote: diff --git a/include/asm-generic/cmpxchg-local.h b/include/asm-generic/cmpxchg-local.h index d8d4c89..4c41bb8 100644 --- a/include/asm-generic/cmpxchg-local.h +++ b/include/asm-generic/cmpxchg-local.h @@ -41,6 +41,7 @@ static inline unsigned long __cmpxchg_local_generic(volatile void *ptr, break; default: wrong_size_cmpxchg(ptr); + prev = 0; } raw_local_irq_restore(flags); return prev; How about using __unreachable() instead to annotate the fact that you won't get here? Yours is a _much_ better idea. Will fix. Thanks, Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] mac80211: LLVMLinux: Remove VLAIS usage from mac80211
On 03/19/14 06:51, Johannes Berg wrote:
I'm confused.
On Tue, 2014-03-18 at 20:32 -0700, beh...@converseincode.com wrote:
struct scatterlist assoc, pt, ct[2];
- struct {
- struct aead_request req;
- u8 priv[crypto_aead_reqsize(tfm)];
- } aead_req;
- memset(&aead_req, 0, sizeof(aead_req));
+ char aead_req_data[sizeof(struct aead_request) +
+ crypto_aead_reqsize(tfm)]
+ __aligned(__alignof__(struct aead_request));
This looks fine, though I'd argue the blank lines before/after it
shouldn't be there, and the indentation should be a bit different, but I
was willing to clean that up.
Will fix.
int ieee80211_aes_ccm_decrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
u8 *data, size_t data_len, u8 *mic)
{
struct scatterlist assoc, pt, ct[2];
- struct {
- struct aead_request req;
- u8 priv[crypto_aead_reqsize(tfm)];
- } aead_req;
- memset(&aead_req, 0, sizeof(aead_req));
+ char aead_req_data[sizeof(struct aead_request) +
+ crypto_aead_reqsize(tfm) +
+ CRYPTO_MINALIGN] CRYPTO_MINALIGN_ATTR;
But why does the second instance use a completely different size/align?
Because I neglected to update it in both places. Sorry. Will fix.
Behan
--
Behan Webster
beh...@converseincode.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] net: netfilter: LLVMLinux: vlais-netfilter
On 03/18/14 08:24, David Laight wrote:
From: Behan Webster
On 03/18/14 02:41, David Laight wrote:
From: beh...@converseincode.com
From: Mark Charlebois
Replaced non-standard C use of Variable Length Arrays In Structs (VLAIS) in
xt_repldata.h with a C99 compliant flexible array member and then calculated
offsets to the other struct members. These other members aren't referenced by
name in this code, however this patch maintains the same memory layout and
padding as was previously accomplished using VLAIS.
Had the original structure been ordered differently, with the entries VLA at
the end, then it could have been a flexible member, and this patch would have
been a lot simpler. However since the data stored in this structure is
ultimately exported to userspace, the order of this structure can't be changed.
Why not just remove the last element and allocate space for it after the
structure?
Because that would still be employing VLAIS to solve the problem. The
last element may be a zero-length array (a flexible member), not a VLA.
Sadly both the last 2 elements in the struct need to be manually
calculated, which is what we've done.
So make the last element a 'flexible member' and then work out where
the final field goes.
Something like:
struct p {
struct a a;
struct b b[];
} p = malloc(sizeof *p + n * sizeof (struct b) + alignof (struct c)
+ sizeof (struct c);
struct c *c = (void *)&p->b[n] + (-offsetof(struct p, b[n]) &
(alignof(struct c) - 1);
Oh, I see. Will fix.
Thanks!
Behan
--
Behan Webster
beh...@converseincode.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] net: netfilter: LLVMLinux: vlais-netfilter
On 03/18/14 02:41, David Laight wrote: From: beh...@converseincode.com From: Mark Charlebois Replaced non-standard C use of Variable Length Arrays In Structs (VLAIS) in xt_repldata.h with a C99 compliant flexible array member and then calculated offsets to the other struct members. These other members aren't referenced by name in this code, however this patch maintains the same memory layout and padding as was previously accomplished using VLAIS. Had the original structure been ordered differently, with the entries VLA at the end, then it could have been a flexible member, and this patch would have been a lot simpler. However since the data stored in this structure is ultimately exported to userspace, the order of this structure can't be changed. Why not just remove the last element and allocate space for it after the structure? Because that would still be employing VLAIS to solve the problem. The last element may be a zero-length array (a flexible member), not a VLA. Sadly both the last 2 elements in the struct need to be manually calculated, which is what we've done. That would reduce the complexity of the patch and the unreadability of the new code. No one is claiming this patch is more readable, merely that it is C99 compliant (though strictly speaking this patch is C89, C99 and C11 compliant). We tried to use macros to make it more readable in previous patches. The consensus was that macros were bad. I realise that the alignment of type##_error is 'tricky' to determine. That is what makes it "unreadable". :( Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] arm: LLVMLinux: use static inline in ARM ftrace.h
On 02/20/14 18:22, beh...@converseincode.com wrote: From: Behan Webster With compilers which follow the C99 standard (like modern versions of gcc and clang), "extern inline" does the wrong thing (emits code for an externally linkable version of the inline function). In this case using static inline and removing the NULL version of return_address in return_address.c does the right thing. Any input? Is it good as it is? Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] module: LLVMLinux: Remove unused function warning from __param_check macro
On 03/10/14 23:11, Rusty Russell wrote: beh...@converseincode.com writes: From: Mark Charlebois This code makes a compile time type check that is optimized away. Clang complains that it generates an unused function. I believe GCC won't complain for a static inline fuction but would if it was just a static function. Adding the unused attribute to the function declaration removes the warning. This code works for both gcc and clang. Signed-off-by: Mark Charlebois Signed-off-by: Behan Webster Please include the actual warning clang spits out. That helps because (1) I know what you're referring to, and (2) it helps others if they are later googling for the error. Nice! Will fix. I don't have any huge objections to this patch (__always_unused) though. Already in the posted v2 patch. However I will post a v3 with your other suggested changes to the commit message. Thanks, Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/5] kbuild: LLVMLinux: Add Kbuild support for building kernel with Clang
On 03/09/14 14:58, Sam Ravnborg wrote: On Tue, Feb 25, 2014 at 05:08:39PM -0800, beh...@converseincode.com wrote: From: Behan Webster Add support to toplevel Makefile for compiling with clang, both for HOSTCC and CC. Use cc-option to prevent gcc option from breaking clang, and from clang options from breaking gcc. Clang 3.4 semantics are the same as gcc semantics for unsupported flags. For unsupported warnings clang 3.4 returns true but shows a warning and gcc shows a warning and returns false. Signed-off-by: Behan Webster Signed-off-by: Jan-Simon Möller Signed-off-by: Mark Charlebois Cc: PaX Team --- Makefile | 32 +++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 831b36a..c4ab30d 100644 --- a/Makefile +++ b/Makefile @@ -247,6 +247,15 @@ HOSTCXX = g++ HOSTCFLAGS = -Wall -Wmissing-prototypes -Wstrict-prototypes -O2 -fomit-frame-pointer HOSTCXXFLAGS = -O2 +ifeq ($(shell $(HOSTCC) -v 2>&1 | grep -c "clang version"), 1) +HOSTCOMPILER := clang +HOSTCFLAGS += -Wno-unused-value -Wno-unused-parameter \ + -Wno-missing-field-initializers -fno-delete-null-pointer-checks +else +HOSTCOMPILER := gcc +endif +export HOSTCOMPILER I see no use of HOSTCOMPLIER anywhere in this patchset not in the kernel. Can we drop this? Actually, we're not using it anymore, though we were at one point. We'll drop it. + # Decide whether to build built-in, modular, or both. # Normally, just do built-in. @@ -323,6 +332,12 @@ endif export quiet Q KBUILD_VERBOSE +ifeq ($(shell $(CC) -v 2>&1 | grep -c "clang version"), 1) +COMPILER := clang +else +COMPILER := gcc +endif +export COMPILER Likewise - COMPILER seems unsued- can it be dropped? We use COMPILER a lot in upcoming patches. :( It's here in order to be available early so that the other patches can use it. I hope that's okay. # Look for make include files relative to root of kernel src MAKEFLAGS += --include-dir=$(srctree) @@ -382,7 +397,7 @@ KBUILD_CFLAGS := -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \ -fno-strict-aliasing -fno-common \ -Werror-implicit-function-declaration \ -Wno-format-security \ - -fno-delete-null-pointer-checks + $(call cc-option,-fno-delete-null-pointer-checks,) KBUILD_AFLAGS_KERNEL := KBUILD_CFLAGS_KERNEL := KBUILD_AFLAGS := -D__ASSEMBLY__ @@ -620,9 +635,24 @@ else endif KBUILD_CFLAGS += $(stackp-flag) +ifeq ($(COMPILER),clang) Except that COMPILER is used here. But this does not warrant the export. Like I said. It is used in at least 4 other patches which are unrelated to kbuild... Well, okay, one in the arm portion of kbuild. I don't actually like using it, and would prefer not to, but we needed the equivalent of an "#ifdef __clang__" in the Makefiles for various reasons. :( +KBUILD_CPPFLAGS += $(call cc-option,-Qunused-arguments,) Is this really needed today? https://bugzilla.mozilla.org/show_bug.cgi?id=717713 suggest that this is default. +KBUILD_CPPFLAGS += $(call cc-option,-Wno-unknown-warning-option,) https://bugzilla.mozilla.org/show_bug.cgi?id=731316 seems to suggest this is default These haven't always been the defaults. I will check on when they became defaults, retest, and reply back. Though thanks for pointing this out! Very thorough. :) +KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable) +KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier) +KBUILD_CFLAGS += $(call cc-disable-warning, gnu) Is it really justified to disable these warnings? # of warnign for a defconfig build would be a nice figure to judge from. To be honest, the number one complaint we get from kernel devs who've tried clang is that there are too many warnings. We don't want to turn them off, but merely to stem the tide for now so as not to put people off. The idea is to turn them on once we've had time to address the warnings more fully. I'd quite like to see all (most) of the clang warnings on by default eventually. Does that make sense? However, the unused-variable warning will likely need to stay on IMHO. There are quite a few things in the kernel which create variables which are intended to be optimized away (last I checked). gcc doesn't complain about this, but clang get's very noisy about it. Though if the consensus is to have more warnings on by default, I'm happy to oblige. +# Quiet clang warning: comparison of unsigned expression < 0 is always false +KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare) Same with this. Actually, if is my understanding that comparing signed and unsigned values are considered valid in the kernel code. Certainly I've seen many patches rejected which were trying to fix "tautological compare
Re: [PATCH v2] mac80211: LLVMLinux: Remove VLAIS usage from mac80211
On 03/08/14 15:00, Sergei Antonov wrote: On 8 March 2014 23:01, PaX Team wrote: On 8 Mar 2014 at 21:29, Sergei Antonov wrote: - memset(&aead_req, 0, sizeof(aead_req)); + char aead_req_data[sizeof(struct aead_request) + + crypto_aead_reqsize(tfm) + + CRYPTO_MINALIGN] CRYPTO_MINALIGN_ATTR; wouldn't the underlined attribute ensure the required alignment? Yes. Sorry, I overlooked it. I would remove unneeded CRYPTO_MINALIGN and get the alignment from the target structure: char aead_req_data[sizeof(struct aead_request) + crypto_aead_reqsize(tfm)] __attribute__((__aligned__(__alignof__(struct aead_request; Even better. Will resubmit. Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] mac80211: LLVMLinux: Remove VLAIS usage from mac80211
On 03/08/14 06:53, Stanislaw Gruszka wrote:
On Fri, Mar 07, 2014 at 06:15:43PM -0800, Behan Webster wrote:
On 03/07/14 17:56, Joe Perches wrote:
On Fri, 2014-03-07 at 17:26 -0800, beh...@converseincode.com wrote:
From: Jan-Simon Möller
Replaced the use of a Variable Length Array In Struct (VLAIS) with a C99
compliant equivalent. This is the original VLAIS struct.
[]
diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
[]
@@ -23,12 +23,14 @@ void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8
*b_0, u8 *aad,
u8 *data, size_t data_len, u8 *mic)
{
struct scatterlist assoc, pt, ct[2];
- struct {
- struct aead_request req;
- u8 priv[crypto_aead_reqsize(tfm)];
- } aead_req;
- memset(&aead_req, 0, sizeof(aead_req));
+ char aead_req_data[sizeof(struct aead_request) +
+ crypto_aead_reqsize(tfm) +
+ CRYPTO_MINALIGN] CRYPTO_MINALIGN_ATTR;
Can this be a too large amount of stack?
Is crypto_aead_reqsize() limited to < ~1k?
Perhaps it'd be better to use kzalloc for this
or another reserved pool
No more stack being used than with the the original code. The stack
memory use is identical.
Could you explain that? It looks like aead_req_data can be bigger than
original struct aead_req up to CRYPTO_MINALIGN bytes. IOW adding
CRYPTO_MINALIGN to size seems unneeded as aead_request->__ctx alignment
requirement should by assured by proper sizeof(struct aead_request).
True, possibly a few more bytes. Nothing significant. However, I will
fix this too.
Besides, why not add VLAIS feature to llvm instead of fixing all
programs that use it?
You aren't the first to ask this (I certainly get asked this regularly). :)
VLAIS is specifically disallowed by the C89, C99, and later C standards.
VLAIS is also not an officially documented feature of gcc (It is a side
effect from the support of other languages supported by the gcc
toolchain). The LLVM project won't add VLAIS support for these reasons,
and because it would unnecessarily complicate their compiler
architecture with almost no appreciable gain.
VLAIS is only used in a handful of places in the kernel (almost
exclusively in crypto related code), so changing it in the kernel not
only is easier, but also makes the kernel code C99 (and beyond)
compliant. Being standards compliant is important not only for clang,
but also for newer versions of gcc (though not yet in the case of VLAIS).
VLAIS should not be confused with Variable Length Arrays (VLA) and
flexible members (undefined or zero length arrays at the end of a non
embedded struct) which are both explicitly in the C standards and
supported by both gcc and clang.
Behan
--
Behan Webster
beh...@converseincode.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] mac80211: LLVMLinux: Remove VLAIS usage from mac80211
On 03/08/14 12:29, Sergei Antonov wrote:
On 8 March 2014 02:26, wrote:
diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
index 7c7df47..3317578 100644
--- a/net/mac80211/aes_ccm.c
+++ b/net/mac80211/aes_ccm.c
@@ -23,12 +23,14 @@ void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8
*b_0, u8 *aad,
u8 *data, size_t data_len, u8 *mic)
{
struct scatterlist assoc, pt, ct[2];
- struct {
- struct aead_request req;
- u8 priv[crypto_aead_reqsize(tfm)];
- } aead_req;
- memset(&aead_req, 0, sizeof(aead_req));
+ char aead_req_data[sizeof(struct aead_request) +
+ crypto_aead_reqsize(tfm) +
+ CRYPTO_MINALIGN] CRYPTO_MINALIGN_ATTR;
+
+ struct aead_request *aead_req = (void *) aead_req_data;
Bad trick with regards to alignment.
The alignment requirement for struct aead_request is stronger than
what an array of chars may have.
Damn it. I should have seen that too. We had to do that in our related
netfilter patch.
Good catch. Will fix.
Thanks,
Behan
--
Behan Webster
beh...@converseincode.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] mac80211: LLVMLinux: Remove VLAIS usage from mac80211
On 03/07/14 18:27, Joe Perches wrote:
On Fri, 2014-03-07 at 18:15 -0800, Behan Webster wrote:
On 03/07/14 17:56, Joe Perches wrote:
On Fri, 2014-03-07 at 17:26 -0800, beh...@converseincode.com wrote:
From: Jan-Simon Möller
Replaced the use of a Variable Length Array In Struct (VLAIS) with a C99
compliant equivalent. This is the original VLAIS struct.
[]
diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
[]
@@ -23,12 +23,14 @@ void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8
*b_0, u8 *aad,
u8 *data, size_t data_len, u8 *mic)
{
struct scatterlist assoc, pt, ct[2];
- struct {
- struct aead_request req;
- u8 priv[crypto_aead_reqsize(tfm)];
- } aead_req;
- memset(&aead_req, 0, sizeof(aead_req));
+ char aead_req_data[sizeof(struct aead_request) +
+ crypto_aead_reqsize(tfm) +
+ CRYPTO_MINALIGN] CRYPTO_MINALIGN_ATTR;
Can this be a too large amount of stack?
Is crypto_aead_reqsize() limited to < ~1k?
Perhaps it'd be better to use kzalloc for this
or another reserved pool
No more stack being used than with the the original code. The stack
memory use is identical.
I do understand that, but that's not my question.
I appreciate you're getting this to compile for llvm.
Any idea of the max value of crypto_aead_reqsize?
And I understand your question, as well as why it is important.
Moving it from being stack based to alloacted memory may or may not be a
good thing, but is orthogonal to the intention of this particular patch
(which is just to remove the use of VLAIS from this code).
$ grep-2.5.4 -rP --include=*.[ch] "(?:->|\.)\s*\breqsize\s*=[^=][^;]+;" *
Very clever. I'm going to use this. :)
Behan
--
Behan Webster
beh...@converseincode.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] module: LLVMLinux: Remove unused function warning from __param_check macro
On 03/07/14 18:17, Joe Perches wrote:
On Fri, 2014-03-07 at 18:10 -0800, beh...@converseincode.com wrote:
This code makes a compile time type check that is optimized away. Clang
complains that it generates an unused function.
[]
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
[]
@@ -346,7 +346,7 @@ static inline void destroy_params(const struct kernel_param
*params,
/* The macros to do compile-time type checking stolen from Jakub
Jelinek, who IIRC came up with this idea for the 2.4 module init code. */
#define __param_check(name, p, type) \
- static inline type *__check_##name(void) { return(p); }
+ static inline __always_unused type *__check_##name(void) { return(p); }
Perhaps __maybe_unused ?
I thought about that (and even tested with __maybe_unused), but I
*think* they are always unused, except at compile time (see comment
above). Though I could be wrong.
I'm certainly okay with __maybe_unused if that is preferable.
Behan
--
Behan Webster
beh...@converseincode.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] mac80211: LLVMLinux: Remove VLAIS usage from mac80211
On 03/07/14 17:56, Joe Perches wrote:
On Fri, 2014-03-07 at 17:26 -0800, beh...@converseincode.com wrote:
From: Jan-Simon Möller
Replaced the use of a Variable Length Array In Struct (VLAIS) with a C99
compliant equivalent. This is the original VLAIS struct.
[]
diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
[]
@@ -23,12 +23,14 @@ void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8
*b_0, u8 *aad,
u8 *data, size_t data_len, u8 *mic)
{
struct scatterlist assoc, pt, ct[2];
- struct {
- struct aead_request req;
- u8 priv[crypto_aead_reqsize(tfm)];
- } aead_req;
- memset(&aead_req, 0, sizeof(aead_req));
+ char aead_req_data[sizeof(struct aead_request) +
+ crypto_aead_reqsize(tfm) +
+ CRYPTO_MINALIGN] CRYPTO_MINALIGN_ATTR;
Can this be a too large amount of stack?
Is crypto_aead_reqsize() limited to < ~1k?
Perhaps it'd be better to use kzalloc for this
or another reserved pool
No more stack being used than with the the original code. The stack
memory use is identical.
Behan
--
Behan Webster
beh...@converseincode.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] module: LLVMLinux: Remove unused function warning from __param_check macro
On 03/07/14 14:56, PaX Team wrote:
On 7 Mar 2014 at 11:08, beh...@converseincode.com wrote:
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index c3eb102..5ce1f67 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -346,6 +346,7 @@ static inline void destroy_params(const struct kernel_param
*params,
/* The macros to do compile-time type checking stolen from Jakub
Jelinek, who IIRC came up with this idea for the 2.4 module init code. */
#define __param_check(name, p, type) \
+ static inline type *__check_##name(void) __attribute__ ((unused)); \
static inline type *__check_##name(void) { return(p); }
why can't you have the attr on the definition itself:
static inline __unused type *__check_##name(void) { return(p); }
"__unused" isn't defined anywhere I can find, except in:
src/linux/drivers/net/ethernet/amd/declance.c:#define __unused
__attribute__ ((unused))
But this does work.
- static inline type *__check_##name(void) { return(p); }
+ static inline __attribute__((unused)) \
+ type *__check_##name(void) { return(p); }
If this is preferred, I will resubmit.
Behan
--
Behan Webster
beh...@converseincode.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/5] kbuild: LLVMLinux: Adapt warnings for compilation with clang
On 02/25/14 17:17, Dave Jones wrote: On Tue, Feb 25, 2014 at 05:08:40PM -0800, beh...@converseincode.com wrote: > When compiling kernel with clang, disable warnings which are too noisy, and > add the clang flag catch-undefined-behavior. > > +# Clang > +warning-1 += $(call cc-disable-warning, initializer-overrides) > +warning-1 += $(call cc-disable-warning, unused-value) > +warning-1 += $(call cc-disable-warning, format) > +warning-1 += $(call cc-disable-warning, unknown-warning-option) > +warning-1 += $(call cc-disable-warning, self-assign) > +warning-1 += $(call cc-disable-warning, sign-compare) > +warning-1 += $(call cc-disable-warning, format-zero-length) > +warning-1 += $(call cc-disable-warning, uninitialized) > +warning-1 += $(call cc-option, -fcatch-undefined-behavior) Do you have a pointer to an example log-file from before this change ? I'm curious for eg, which self-assign warnings are showing up, because I've been fixing up the ones that Coverity found, of which there are only a dozen or so left iirc. I count 22 in this particular x86 kernel build (some of which you may well have fixed already). Enjoy. http://buildbot.llvm.linuxfoundation.org/self-assign-build.log.txt Perhaps I should re-enable that warning, considering the number has dropped so dramatically from when I last checked over a year ago. Thanks, Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/5] x86 kbuild: LLVMLinux: More cc-options added for clang
On 02/25/14 17:32, H. Peter Anvin wrote: On 02/25/2014 05:08 PM, beh...@converseincode.com wrote: +# enforce no-sse for clang +ifneq ($(COMPILER),clang) +KBUILD_CFLAGS += $(call cc-option,-mno-sse) +KBUILD_CFLAGS += $(call cc-option,-mpreferred-stack-boundary=3) +endif + I'm *very* confused. You're doing ifneq here but you're talking about it as if you are *adding* them for Clang, also these options are already added elsewhere (lines 57, 64, 86, 89) so why add them here? Because when David Woodhouse's recent .code16 changes made it upstream (which invalidated most of our original patch) it seems I didn't remove this properly from our patch as well. :) Thanks David for your patches. Thanks Peter for your sharp eyes! Another example of why reviewing code on the mailing list works so well. I will fix and resend. Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/5] kbuild: LLVMLinux: Adapt warnings for compilation with clang
On 02/25/14 17:17, Dave Jones wrote: On Tue, Feb 25, 2014 at 05:08:40PM -0800, beh...@converseincode.com wrote: > +warning-1 += $(call cc-disable-warning, self-assign) Do you have a pointer to an example log-file from before this change ? I'm curious for eg, which self-assign warnings are showing up, because I've been fixing up the ones that Coverity found, of which there are only a dozen or so left iirc. Some of the others may also be interesting. I don't have one now, but I can generate a log and send it to you. We've had that option set from near the beginning of the porting effort. This are primarily off due to the amount of noise these warnings produce at this time (clang produces a LOT of warnings straight out of the box). I'd love to eventually turn some of these back on again. Thanks, Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] module: LLVMLinux: Fix section mismatch issues on alias usage
On 02/21/14 07:08, Paul Gortmaker wrote: Attribute aliases don't inherit the link section name when compiled with clang. As a result, the linking section needs to be explicitly specified when building a module. This behavior is undefined in the standard which is why it differs from compiler to compiler. But is there a good reason why clang doesn't inherit them in the interest of compatibility with gcc and existing code? There is no reason other than it doesn't appear to be documented behaviour anywhere that can be found. It seems like this use of an aliased symbol inheriting attributes never came up before. Personally I think the kernel code pushes compilers harder than most other code bases. :) Having said that, it seems since we first started needing this patch to build the kernel with clang, this issue may have been fixed in the most recent updates to clang in the last few weeks. I've been testing with the latest released version of clang v3.4 (since clang recently when through a merge window of their own) which still requires this patch. However in appears that this issue may now have been fixed in mainline clang. I'll do some more testing to verify and get back to this thread. I know we've seen the faceless entity "PaX Team" before, but can we please not make it worse by adding a bunch of other non standard tag line formats? And ideally have a real human name for the author too. Fair enough. Will fix and resubmit if the patch proves to still be necessary. Thanks for the input, Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86, acpi: LLVMLinux: Remove nested functions from Thinkpad ACPI
On 02/12/14 13:11, Christoph Hellwig wrote: On Wed, Feb 12, 2014 at 09:58:46PM +0100, dl...@gmx.de wrote: being able to compile the Linux kernel with Clang. The use of nested functions blocks this effort. Is there any good way to make gcc warn about the use of nested functions? Interesting idea. '-Wtrampolines' Warn about trampolines generated for pointers to nested functions. A trampoline is a small piece of data or code that is created at run time on the stack when the address of a nested function is taken, and is used to call the nested function indirectly. For some targets, it is made up of data only and thus requires no special treatment. But, for most targets, it is made up of code and thus requires the stack to be made executable in order for the program to work properly. That might work. Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Remove VLAIS usage from gadget code - alternate patch
On 09/24/13 13:56, charl...@gmail.com wrote: From: Mark Charlebois The use of variable length arrays in structs (VLAIS) in the Linux Kernel code precludes the use of compilers which don't implement VLAIS (for instance the Clang compiler). This alternate patch calculates offsets into the kmalloc-ed memory buffer using macros. The previous patch required multiple kmalloc and kfree calls. Signed-off-by: Mark Charlebois Signed-off-by: Behan Webster This one uses macros and carves up a single piece of memory from one kmalloc. Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: LLVMLinux: Remove VLAIS from USB gadget
On 09/23/13 18:53, charl...@gmail.com wrote: From: Mark Charlebois The use of variable length arrays in structs (VLAIS) in the Linux Kernel code precludes the use of compilers which don't implement VLAIS (for instance the Clang compiler). This patch removes the use of VLAIS in the gadget driver. This version has been tested to compile cleanly. Signed-off-by: Mark Charlebois Signed-off-by: Behan Webster This is the fixed patch which was sent to Andrzej Pietrasiewicz. Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] USB: Removing the use of VLAIS from the gadget driver
On 09/23/13 16:24, Felipe Balbi wrote:
Hi,
On Thu, Aug 01, 2013 at 09:35:44PM -0400, beh...@converseincode.com wrote:
From: Behan Webster
The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
precludes the use of compilers which don't implement VLAIS (for instance the
Clang compiler). This patch removes the use of VLAIS in the gadget driver.
Signed-off-by: Mark Charlebois
Signed-off-by: Behan Webster
---
drivers/usb/gadget/f_fs.c | 128 +++---
1 file changed, 76 insertions(+), 52 deletions(-)
diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index f394f29..4b872c4 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -30,7 +30,6 @@
#define FUNCTIONFS_MAGIC 0xa647361 /* Chosen by a honest dice roll ;) */
-
/* Debugging /
#ifdef VERBOSE_DEBUG
@@ -214,6 +213,8 @@ struct ffs_data {
/* ids in stringtabs are set in functionfs_bind() */
const void *raw_strings;
struct usb_gadget_strings **stringtabs;
+ struct usb_gadget_strings *stringtab;
+ struct usb_string *strings;
/*
* File system's super block, write once when file system is
@@ -263,7 +264,10 @@ struct ffs_function {
struct ffs_ep *eps;
u8 eps_revmap[16];
+ struct usb_descriptor_header**fs_descs;
+ struct usb_descriptor_header**hs_descs;
short *interfaces_nums;
+ char*raw_descs;
struct usb_function function;
};
@@ -1345,6 +1349,8 @@ static void ffs_data_clear(struct ffs_data *ffs)
kfree(ffs->raw_descs);
kfree(ffs->raw_strings);
kfree(ffs->stringtabs);
+ kfree(ffs->stringtab);
+ kfree(ffs->strings);
}
static void ffs_data_reset(struct ffs_data *ffs)
@@ -1357,6 +1363,8 @@ static void ffs_data_reset(struct ffs_data *ffs)
ffs->raw_descs = NULL;
ffs->raw_strings = NULL;
ffs->stringtabs = NULL;
+ ffs->stringtab = NULL;
+ ffs->strings = NULL;
ffs->raw_descs_length = 0;
ffs->raw_fs_descs_length = 0;
@@ -1528,12 +1536,10 @@ static void ffs_func_free(struct ffs_function *func)
ffs_data_put(func->ffs);
kfree(func->eps);
- /*
-* eps and interfaces_nums are allocated in the same chunk so
-* only one free is required. Descriptors are also allocated
-* in the same chunk.
-*/
-
+ kfree(func->fs_descs);
+ kfree(func->hs_descs);
+ kfree(func->interfaces_nums);
+ kfree(func->raw_descs);
kfree(func);
}
@@ -1907,33 +1913,35 @@ static int __ffs_data_got_strings(struct ffs_data *ffs,
return 0;
}
- /* Allocate everything in one chunk so there's less maintenance. */
{
- struct {
- struct usb_gadget_strings *stringtabs[lang_count + 1];
- struct usb_gadget_strings stringtab[lang_count];
- struct usb_string strings[lang_count*(needed_count+1)];
- } *d;
unsigned i = 0;
-
- d = kmalloc(sizeof *d, GFP_KERNEL);
- if (unlikely(!d)) {
+ usb_gadget_strings **stringtabs = NULL;
+ usb_gadget_strings *stringtab = NULL;
+ usb_string *strings = NULL;
did you compile this patch ?
Appologies. The patch as posted has a bug which was fixed after sending
it to the list.
Andrzej Pietrasiewicz has the fixed one. Will
send the fixed one to the list too.
Behan
--
Behan Webster
beh...@converseincode.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: gadget LLVMLinux: Removing the use of VLAIS from the gadget driver
On 09/23/13 15:08, Linus Torvalds wrote: On Mon, Sep 23, 2013 at 12:30 PM, Felipe Balbi wrote: I remember there was a discussion of not dropping variable length array support, wasn't there ? We should definitely drop it. The feature is an abomination. I thought gcc only allowed them at the end of structs, in the middle of a struct it's just f*cking insane beyond belief. That said, for *this* particular case, that USB widget driver already does a ton of small kmalloc's and then remembers the addresses independently. People may not care about performance, but it *might* be a good idea to just do one kmalloc()/kfree(), and then still have those pointer variables, but just be offsets within that one allocation. That's what gcc has to basically do for that thing internally *anyway*, just hidden behind a horrible construct that should never have existed. We can certainly do that instead. I believe I already have a version of the patch which does just that (without using macros). I will post it for comment. Thanks, Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: gadget LLVMLinux: Removing the use of VLAIS from the gadget driver
On 09/23/13 14:30, Felipe Balbi wrote: Hi, On Thu, Sep 05, 2013 at 08:07:11PM -0400, Behan Webster wrote: Replying to my patch email just in case it was missed before. I remember there was a discussion of not dropping variable length array support, wasn't there ? There was mostly an objection to the implementation of that patch. The new patch follows the advice given by the linux-usb list members at that time. There is increasing interest from various kernel devs (including maintainers and above) to be able to use clang to compile the Linux kernel. The removal of the use of VLAIS in the kernel is a step in allowing that to happen. Andrzej Pietrasiewicz attended the LLVM microconference at Plumbers last week and accepted the USB gadget patch. Thanks, Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: LLVMLinux: Change "extern inline" to "gnu_inline" in ARM ftrace.h
Sorry for the delay. A mistake in my email filters ate all your replies. Doh! On 08/14/13 18:45, Russell King - ARM Linux wrote: On Wed, Aug 14, 2013 at 05:37:41PM -0400, beh...@converseincode.com wrote: -extern inline void *return_address(unsigned int level) +extern inline __attribute__((gnu_inline)) +void *return_address(unsigned int level) Well, that should be static inline, not extern inline in any case. Does clang work if that's static inline? Actually, neither gcc nor clang work with it merely changed to "static inline". Which is why we left it with the explicit GNU89 meaning of "extern inline" which is gnu_inline. C99 changed the meaning of what "extern inline" means. One of the major issues we've had with the clang kernel port is that clang defaults to gnu99 (which is mostly just C99) while until recently gcc defaulted to gnu89. For recent versions of gcc: http://gcc.gnu.org/onlinedocs/gcc/Standards.html "The default, if no C language dialect options are given, is -std=gnu90; this will change to -std=gnu99 or -std=gnu11 in some future release when the C99 or C11 support is complete. Some features that are part of the C99 standard are accepted as extensions in C90 mode, and some features that are part of the C11 standard are accepted as extensions in C90 and C99 modes." However, having said all that, it seems if I remove the corresponding NULL definition for return_address in arch/arm/kernel/return_address.c, I can make it "static inline" and it seems to work for both gcc and clang. I'll send a new patch. :) Incidentally the LLVMLinux project tests all the project's patches with both gcc and clang. The idea is to make it work with both compilers after all. Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: gadget LLVMLinux: Removing the use of VLAIS from the gadget driver
Replying to my patch email just in case it was missed before.
Thanks,
Behan
On 08/01/13 21:35, beh...@converseincode.com wrote:
From: Behan Webster
The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
precludes the use of compilers which don't implement VLAIS (for instance the
Clang compiler). This patch removes the use of VLAIS in the gadget driver.
Signed-off-by: Mark Charlebois
Signed-off-by: Behan Webster
---
drivers/usb/gadget/f_fs.c | 128 +++---
1 file changed, 76 insertions(+), 52 deletions(-)
diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index f394f29..4b872c4 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -30,7 +30,6 @@
#define FUNCTIONFS_MAGIC 0xa647361 /* Chosen by a honest dice roll ;) */
-
/* Debugging /
#ifdef VERBOSE_DEBUG
@@ -214,6 +213,8 @@ struct ffs_data {
/* ids in stringtabs are set in functionfs_bind() */
const void *raw_strings;
struct usb_gadget_strings **stringtabs;
+ struct usb_gadget_strings *stringtab;
+ struct usb_string *strings;
/*
* File system's super block, write once when file system is
@@ -263,7 +264,10 @@ struct ffs_function {
struct ffs_ep *eps;
u8 eps_revmap[16];
+ struct usb_descriptor_header**fs_descs;
+ struct usb_descriptor_header**hs_descs;
short *interfaces_nums;
+ char*raw_descs;
struct usb_function function;
};
@@ -1345,6 +1349,8 @@ static void ffs_data_clear(struct ffs_data *ffs)
kfree(ffs->raw_descs);
kfree(ffs->raw_strings);
kfree(ffs->stringtabs);
+ kfree(ffs->stringtab);
+ kfree(ffs->strings);
}
static void ffs_data_reset(struct ffs_data *ffs)
@@ -1357,6 +1363,8 @@ static void ffs_data_reset(struct ffs_data *ffs)
ffs->raw_descs = NULL;
ffs->raw_strings = NULL;
ffs->stringtabs = NULL;
+ ffs->stringtab = NULL;
+ ffs->strings = NULL;
ffs->raw_descs_length = 0;
ffs->raw_fs_descs_length = 0;
@@ -1528,12 +1536,10 @@ static void ffs_func_free(struct ffs_function *func)
ffs_data_put(func->ffs);
kfree(func->eps);
- /*
-* eps and interfaces_nums are allocated in the same chunk so
-* only one free is required. Descriptors are also allocated
-* in the same chunk.
-*/
-
+ kfree(func->fs_descs);
+ kfree(func->hs_descs);
+ kfree(func->interfaces_nums);
+ kfree(func->raw_descs);
kfree(func);
}
@@ -1907,33 +1913,35 @@ static int __ffs_data_got_strings(struct ffs_data *ffs,
return 0;
}
- /* Allocate everything in one chunk so there's less maintenance. */
{
- struct {
- struct usb_gadget_strings *stringtabs[lang_count + 1];
- struct usb_gadget_strings stringtab[lang_count];
- struct usb_string strings[lang_count*(needed_count+1)];
- } *d;
unsigned i = 0;
-
- d = kmalloc(sizeof *d, GFP_KERNEL);
- if (unlikely(!d)) {
+ usb_gadget_strings **stringtabs = NULL;
+ usb_gadget_strings *stringtab = NULL;
+ usb_string *strings = NULL;
+
+ stringtabs = kmalloc(sizeof(*stringtabs)*(lang_count + 1),
+ GFP_KERNEL);
+ stringtab = kmalloc(sizeof(*stringtab)*(lang_count),
+ GFP_KERNEL);
+ strings = kmalloc(sizeof(*strings)
+ * (lang_count * (needed_count + 1)), GFP_KERNEL);
+ if (unlikely(!stringtabs || !stringtab || !strings)) {
+ kfree(stringtabs);
+ kfree(stringtab);
+ kfree(strings);
kfree(_data);
return -ENOMEM;
}
-
- stringtabs = d->stringtabs;
- t = d->stringtab;
+ b = stringtabs;
+ t = stringtab;
i = lang_count;
do {
- *stringtabs++ = t++;
+ *b++ = t++;
} while (--i);
- *stringtabs = NULL;
+ *b = NULL;
- stringtabs = d->stringtabs;
- t = d->stringtab;
- s = d->strings;
- strings = s;
+ t = stringtab;
+ s = strings;
}
/* For each language */
@@ -1991,12 +1999,16 @@ static int __ffs_data_got_strings(struct ffs_data *ffs,
/* Done! */
ff
Re: [PATCH] slab: Remove unnecessary __builtin_constant_p()
On 13-04-22 03:58 PM, Steven Rostedt wrote: On Mon, 2013-04-22 at 13:44 -0700, Andrew Morton wrote: On Wed, 17 Apr 2013 17:03:21 -0700 (PDT) David Rientjes wrote: On Wed, 17 Apr 2013, Steven Rostedt wrote: The slab.c code has a size check macro that checks the size of the following structs: struct arraycache_init struct kmem_list3 The index_of() function that takes the sizeof() of the above two structs and does an unnecessary __builtin_constant_p() on that. As sizeof() will always end up being a constant making this always be true. The code is not incorrect, but it just adds added complexity, and confuses users and wastes the time of reviewers of the code, who spends time trying to figure out why the builtin_constant_p() was used. This patch is just a clean up that makes the index_of() code a little bit less complex. Signed-off-by: Steven Rostedt Acked-by: David Rientjes Adding Pekka to the cc. I ducked this patch because it seemed rather pointless - but a little birdie told me that there is a secret motivation which seems pretty reasonable to me. So I shall await chirp-the-second, which hopefully will have a fuller and franker changelog ;) The real motivation behind this patch was it prevents LLVM (Clang) from compiling the kernel. There's currently a bug in Clang where it can't determine if a variable is constant or not, so instead, when __builtin_constant_p() is used, it just treats it like it isn't a constant (always taking the slow *safe* path). Unfortunately, the "confusing" code of slub.c that unnecessarily uses the __builtin_constant_p() will fail to compile if the variable passed in is not constant. As Clang will say constants are not constant at this point, the compile fails. When looking into this, we found the only two users of the index_of() static function that has this issue, passes in size_of(), which will always be a constant, making the check redundant. Note, this is a bug in Clang that will hopefully be fixed soon. But for now, this strange redundant compile time check is preventing Clang from even testing the Linux kernel build. And I still think the original change log has rational for the change, as it does make it rather confusing to what is happening there. -- Steve Just to pipe up since Steve was helping me out with this patch. I just want to make it clear that in no way am I trying to sneak any code into the kernel in order to merely support Clang (certainly the motivation for the patch wasn't meant to be a secret). That in this case the code might be considered clearer at the same time as enabling Clang to be used to compile this portion of code seemed to be a win-win situation to me. I certainly thank Steve, Christoph and Andrew for their support in principle in this particular matter (not that it is yet a done deal). I merely complained about this particular issue at my talk at the recent Collab Summit and Steve jumped in to follow up with this particular solution as well as connecting up myself with the three of them (all of us being in the same hotel in San Francisco at the same time). My god Steve works fast! It made my head spin. My motivation (as a part of the LLVMLinux project) is purely to provide another choice of toolchain to the kernel developer and system integrator, some of whom would like the choice of using (or at least trying) Clang. I certainly do not intentionally want to negatively impact the performance nor code quality of the kernel code base to the best of my ability (quite the opposite actually). I think I can safely say that the competition between the 2 toolchains has already made both even stronger than they were previously (certainly gcc 4.8 and the upcoming LLVM/Clang 3.3 seem to be the best either have ever been). As far as __builtin_constant_p() in clang goes, it gets it right in many places (i.e. agrees with how gcc evaluates it), but in this particular situation it got it wrong. However, in this case I was having troubles understanding why __builtin_constant_p() was being used the way it was in slab.c at all... Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2 2/3] Remove VLAIS usage from gadget code
On 12-12-04 11:24 PM, Sebastian Andrzej Siewior wrote: On Mon, Dec 03, 2012 at 07:57:33PM +0100, Behan Webster wrote: However, in order to approximate what gcc is doing in code, obviously some math is required. The thought was that macros would hide the worst of it, trying not to obfuscate what was actually being done. Why hide? The thing that is done here is setting up pointers and keep this struct as a container. It is never used again, just freed. Therefore I would suggest to remove and do something different. Fair enough. That works too. I've been profiling some sample code around this implementation comparing it between compilers, and it approximates the code size and speed of using VLAIS in gcc (especially with -O2). It actually performs better than the previously proposed macros. I'm not concerned about speed here. This is an one time setup. Which is all good in the case of the gadget code. However VLAIS is used in other places too where code size and speed are very important. The proposal was for something which might be used for more than just the gadget code. VLAIS is used in only a handful of places in the kernel. They just happen to be in important parts of the kernel which was why the approach was to change as little as necessary. But certainly if anyone has a solution which works for everyone, then I'm more than happy to adopt it. The LLVM community has made quite a few changes in order to help get Linux working with clang. However, That is nice to hear. Besides gcc there is the icc. Lots of people have brought up icc. Although in the same breath most also say nobody is using it anymore. However, I have no idea whether that is true. The difference is that clang is widely available in most distros; icc is not. VLAIS is not something they are willing to accept (for various reasons). There are other patches to LLVM that are still working Is this not described in C99 6.7.2.1p16? You're thinking of "Flexible Array Members". Essentially the last element in a structure can be the equivalent of a zero length array; something which both gcc and clang support today. From C99 6.7.2.1p16: 16 As a special case, the last element of a structure with more than one named member may have an incomplete array type; this is called a flexible array member. In most situations, the flexible array member is ignored. In particular, the size of the structure is as if the flexible array member were omitted except that it may have more trailing padding than the omission would imply. Howev er, when a . (or ->) operator has a left operand that is (a pointer to) a structure with a flexible array member and the right operand names that member, it behaves as if that member were replaced with the longest array (with the same element type) that would not make the structure larger than the object being accessed; the offset of the array shall remain that of the flexible array member, even if this would differ from that of the replacement array. If this array would have no elements, it behaves as if it had one element but the behavior is undefined if any attempt is made to access that element or to generate a pointer one past it. However if we're considering the C99 standard, it also explicitly disallows the use of VLAs in structs (VLAIS). From C99 6.7.2.1 p2: 2 A structure or union shall not contain a member with incomplete or function type (hence, a structure shall not contain an instance of itself, but may contain a pointer to an instance of itself), except that the last member of a structure with more than one named member may have incomplete array type; such a structure (and any union containing, possibly recursively, a member that is such a structure) shall not be a member of a structure or an element of an array. From C99 6.7.5.2 p4: 4 If the size is not present, the array type is an incomplete type. Essentially all arrays in a structure must not be variable length except the last member of the array which can be a zero length array (which can be used to approximate a VLA at the end). It also says that a struct which uses a flexible array member can't be used inside another struct. their way upstream that are required to be able to compile Linux as well. I hope the other are "simple" to get in :) I hope so too. But like the Linux kernel community, the LLVM community have standards and procedures for getting code accepted which take time. Thanks, Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2 2/3] Remove VLAIS usage from gadget code
On 12-12-03 12:57 PM, Sebastian Andrzej Siewior wrote: On Thu, Nov 01, 2012 at 09:21:16AM +0200, Felipe Balbi wrote: then we can merge to net tree and handle the conflicts when merging to Linus, that'd be fine by me as long as people know how to solve the conflict properly ;-) Felipe please drop this patch. I don't like this VLAIS patch and its macro magic. I support the goal of compiling the kernel with clang but I don't think that this is appropriate. Davem has also his problems with the netfilter part so I am no the only one. You are definitely not the only one. :) But that's okay. The idea is to find an alternative that works for everyone. It was the first attempt, which didn't work out. *I* think the gadget part of the kernel is something most people won't use so it won't show up. Anyway, please drop this completely I try to think of something sane. He actually didn't take the patch. He was asking the netfilter team to take it, and they didn't want to. Fair enough. However, in order to approximate what gcc is doing in code, obviously some math is required. The thought was that macros would hide the worst of it, trying not to obfuscate what was actually being done. One of the project members came up with this alternative. How about something like this? Less math, though more string pasting. When compiled, the unused variables get optimized away. Otherwise the memory packing is identical to using VLAIS in gcc. #define vla_struct(structname) size_t structname##__##next = 0 #define vla_struct_size(structname) structname##__##next #define vla_item(structname, type, name, n) \ type * structname##_##name; \ size_t structname##_##name##__##pad = \ (structname##__##next & (__alignof__(type)-1)); \ size_t structname##_##name##__##offset = \ structname##__##next + structname##_##name##__##pad; \ size_t structname##_##name##__##sz = n * sizeof(type); \ structname##__##next = structname##__##next + \ structname##_##name##__##pad + structname##_##name##__##sz; #define vla_ptr(ptr,structname,name) structname##_##name = \ (typeof(structname##_##name))&ptr[structname##_##name##__##offset] Then you can do something like this that looks vaguely struct-like: vla_struct(foo); vla_item(foo, char, vara, 1); vla_item(foo, short, varb, 10); vla_item(foo, int, varc, 5); vla_item(foo, long, vard, 3); size_t total = vla_struct_size(foo); char buffer[total]; vla_ptr(buffer, foo, varc); foo_varc = 1; I've been profiling some sample code around this implementation comparing it between compilers, and it approximates the code size and speed of using VLAIS in gcc (especially with -O2). It actually performs better than the previously proposed macros. But certainly if anyone has a solution which works for everyone, then I'm more than happy to adopt it. The LLVM community has made quite a few changes in order to help get Linux working with clang. However, VLAIS is not something they are willing to accept (for various reasons). There are other patches to LLVM that are still working their way upstream that are required to be able to compile Linux as well. As far as I've been told (though I have been unable to verify), Variable Length Arrays In Structs (VLAIS) is a feature which made it into gcc along with nested functions by the work on the Ada frontend. By contrast, the C99 standard specifies Variable Length Arrays (outside of structs) and Flexible Array Members (essentially zero-length arrays at the end of a struct), and as such both are supported by gcc and clang. The LLVMLinux project is a meta-project between the LLVM and Linux communities trying to get the toolchain from one working with the code from the other. The LLVMLinux project isn't trying to replace, nor break gcc (in fact all the project's kernel patches are tested with gcc as well). The idea is to bring another toolchain and set of tools to the table and the kernel community. Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2 2/3] Remove VLAIS usage from gadget code
On 12-10-31 09:28 AM, Felipe Balbi wrote: hi, On Tue, Oct 30, 2012 at 05:18:56PM -0400, Behan Webster wrote: The use of variable length arrays in structs (VLAIS) in the Linux Kernel code precludes the use of compilers which don't implement VLAIS (for instance the Clang compiler). This patch instead calculates offsets into the kmalloc-ed memory buffer using macros from valign.h. Signed-off-by: Behan Webster this won't apply after the current cleanups I applied to gadget code from Sebastian. Makes sense. I'll try it with your repo, and regenerate. If someone takes this patch, it will generate a series of annoying, hard-to-figure-out conflicts (at least judging by the looks of $SUBJECT). I just tried the patch on your git.kernel.org repo and thankfully there is only one hunk which is rejected, and fortunately the reason is trivial (descriptors -> fs_descriptors). Was: -func->function.descriptors = data->fs_descs; +func->function.descriptors = fs_descs; Now is: -func->function.fs_descriptors = data->fs_descs; +func->function.fs_descriptors = fs_descs; I will regenerate the patch set, but obviously the new gadget patch in the V3 patchset will only apply to the USB repo, and not to the netfilter repo. Thanks, Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
