Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit a8668298 authored by Dan Albert's avatar Dan Albert
Browse files

Verify that ndk_headers headers are valid C. 

https://github.com/android/ndk/issues/1920 recently reported a handful
of NDK headers where C-incompatible syntax had slipped in which wasn't
caught by review. This is also something that we often manually catch
in API review, but the manual inspection is, as can be seen from that
bug, error prone.

This check is pretty stupid. I've tried on other occasions to do a
more thorough check that would build each header using the same flags
as cc_library or similar would, but was never able to get that
working. This isn't as thorough, but that's maybe not such a bad
thing, since it saves on build time to only check on variant. In
practice the rudimentary approach I've taken here would have caught
every instance reported in the bug above, and probably all the other
cases that we'd previously caught in API review as well, so it's
definitely a step in the right direction if not good enough forever.

Bug: http://b/113359184
Test: m ndk
Change-Id: Id4e8cc511413acc61c4f625f25c3004d7439263c
parent 3bec2a77

cc/ndk_headers.go

+16 −1
Original line number Diff line number Diff line
@@ -18,6 +18,7 @@ import ( 
	"path/filepath"



	"android/soong/android"



	"github.com/google/blueprint"

)
@@ -44,7 +45,7 @@ func init() { 
}



// Returns the NDK base include path for use with sdk_version current. Usable with -I.

func getCurrentIncludePath(ctx android.ModuleContext) android.OutputPath {

func getCurrentIncludePath(ctx android.PathContext) android.OutputPath {

	return getNdkSysrootBase(ctx).Join(ctx, "usr/include")

}
@@ -73,6 +74,13 @@ type headerProperties struct { 


	// Path to the NOTICE file associated with the headers.

	License *string `android:"path"`



	// Set to true if the headers installed by this module should skip

	// verification. This step ensures that each header is self-contained (can

	// be #included alone) and is valid C. This should not be disabled except in

	// rare cases. Outside bionic and external, if you're using this option

	// you've probably made a mistake.

	Skip_verification *bool

}



type headerModule struct {
@@ -309,6 +317,13 @@ type preprocessedHeadersProperties struct { 


	// Path to the NOTICE file associated with the headers.

	License *string



	// Set to true if the headers installed by this module should skip

	// verification. This step ensures that each header is self-contained (can

	// be #included alone) and is valid C. This should not be disabled except in

	// rare cases. Outside bionic and external, if you're using this option

	// you've probably made a mistake.

	Skip_verification *bool

}



type preprocessedHeadersModule struct {

cc/ndk_sysroot.go

+88 −1
Original line number Diff line number Diff line
@@ -54,7 +54,22 @@ package cc 


import (

	"android/soong/android"

	"fmt"

	"path/filepath"

	"strings"



	"github.com/google/blueprint"

)



var (

	verifyCCompat = pctx.AndroidStaticRule("verifyCCompat",

		blueprint.RuleParams{

			Command:     "$ccCmd -x c -fsyntax-only $flags $in && touch $out",

			CommandDeps: []string{"$ccCmd"},

		},

		"ccCmd",

		"flags",

	)

)



func init() {
@@ -103,6 +118,45 @@ func getNdkABIHeadersFile(ctx android.PathContext) android.WritablePath { 
	return android.PathForOutput(ctx, "ndk_abi_headers.txt")

}



func verifyNdkHeaderIsCCompatible(ctx android.SingletonContext,

	src android.Path, dest android.Path) android.Path {

	sysrootInclude := getCurrentIncludePath(ctx)

	baseOutputDir := android.PathForOutput(ctx, "c-compat-verification")

	installRelPath, err := filepath.Rel(sysrootInclude.String(), dest.String())

	if err != nil {

		ctx.Errorf("filepath.Rel(%q, %q) failed: %s", dest, sysrootInclude, err)

	}

	output := baseOutputDir.Join(ctx, installRelPath)

	ctx.Build(pctx, android.BuildParams{

		Rule:        verifyCCompat,

		Description: fmt.Sprintf("Verifying C compatibility of %s", src),

		Output:      output,

		Input:       dest,

		// Ensures that all the headers in the sysroot are already installed

		// before testing any of the headers for C compatibility, and also that

		// the check will be re-run whenever the sysroot changes. This is

		// necessary because many of the NDK headers depend on other NDK

		// headers, but we don't have explicit dependency tracking for that.

		Implicits: []android.Path{getNdkHeadersTimestampFile(ctx)},

		Args: map[string]string{

			"ccCmd": "${config.ClangBin}/clang",

			"flags": fmt.Sprintf(

				// Ideally we'd check each ABI, multiple API levels,

				// fortify/non-fortify, and a handful of other variations. It's

				// a lot more difficult to do that though, and would eat up more

				// build time. All the problems we've seen so far that this

				// check would catch have been in arch-generic and

				// minSdkVersion-generic code in frameworks though, so this is a

				// good place to start.

				"-target aarch64-linux-android%d --sysroot %s",

				android.FutureApiLevel.FinalOrFutureInt(),

				getNdkSysrootBase(ctx).String(),

			),

		},

	})

	return output

}



func NdkSingleton() android.Singleton {

	return &ndkSingleton{}

}
@@ -143,10 +197,17 @@ func writeNdkAbiSrcFilter(ctx android.BuilderContext, 


type ndkSingleton struct{}



type srcDestPair struct {

	src  android.Path

	dest android.Path

}



func (n *ndkSingleton) GenerateBuildActions(ctx android.SingletonContext) {

	var staticLibInstallPaths android.Paths

	var headerSrcPaths android.Paths

	var headerInstallPaths android.Paths

	var headersToVerify []srcDestPair

	var headerCCompatVerificationTimestampPaths android.Paths

	var installPaths android.Paths

	var licensePaths android.Paths

	ctx.VisitAllModules(func(module android.Module) {
@@ -157,6 +218,14 @@ func (n *ndkSingleton) GenerateBuildActions(ctx android.SingletonContext) { 
		if m, ok := module.(*headerModule); ok {

			headerSrcPaths = append(headerSrcPaths, m.srcPaths...)

			headerInstallPaths = append(headerInstallPaths, m.installPaths...)

			if !Bool(m.properties.Skip_verification) {

				for i, installPath := range m.installPaths {

					headersToVerify = append(headersToVerify, srcDestPair{

						src:  m.srcPaths[i],

						dest: installPath,

					})

				}

			}

			installPaths = append(installPaths, m.installPaths...)

			licensePaths = append(licensePaths, m.licensePath)

		}
@@ -164,6 +233,10 @@ func (n *ndkSingleton) GenerateBuildActions(ctx android.SingletonContext) { 
		if m, ok := module.(*versionedHeaderModule); ok {

			headerSrcPaths = append(headerSrcPaths, m.srcPaths...)

			headerInstallPaths = append(headerInstallPaths, m.installPaths...)

			// Verification intentionally not done for headers that go through

			// versioner. It'd be nice to have, but the only user is bionic, and

			// that one module would also need to use skip_verification, so it

			// wouldn't help at all.

			installPaths = append(installPaths, m.installPaths...)

			licensePaths = append(licensePaths, m.licensePath)

		}
@@ -171,6 +244,14 @@ func (n *ndkSingleton) GenerateBuildActions(ctx android.SingletonContext) { 
		if m, ok := module.(*preprocessedHeadersModule); ok {

			headerSrcPaths = append(headerSrcPaths, m.srcPaths...)

			headerInstallPaths = append(headerInstallPaths, m.installPaths...)

			if !Bool(m.properties.Skip_verification) {

				for i, installPath := range m.installPaths {

					headersToVerify = append(headersToVerify, srcDestPair{

						src:  m.srcPaths[i],

						dest: installPath,

					})

				}

			}

			installPaths = append(installPaths, m.installPaths...)

			licensePaths = append(licensePaths, m.licensePath)

		}
@@ -223,6 +304,12 @@ func (n *ndkSingleton) GenerateBuildActions(ctx android.SingletonContext) { 
		Implicits: headerInstallPaths,

	})



	for _, srcDestPair := range headersToVerify {

		headerCCompatVerificationTimestampPaths = append(

			headerCCompatVerificationTimestampPaths,

			verifyNdkHeaderIsCCompatible(ctx, srcDestPair.src, srcDestPair.dest))

	}



	writeNdkAbiSrcFilter(ctx, headerSrcPaths, getNdkABIHeadersFile(ctx))



	fullDepPaths := append(staticLibInstallPaths, getNdkBaseTimestampFile(ctx))
@@ -235,6 +322,6 @@ func (n *ndkSingleton) GenerateBuildActions(ctx android.SingletonContext) { 
	ctx.Build(pctx, android.BuildParams{

		Rule:      android.Touch,

		Output:    getNdkFullTimestampFile(ctx),

		Implicits: fullDepPaths,

		Implicits: append(fullDepPaths, headerCCompatVerificationTimestampPaths...),

	})

}