AAPT2: Record public status in a more robust way

This allows us to store the source and comments of a resource's
public declaration and avoids issues where there is no default
configuration for a publicly declared resource (like with drawables
of various densities) and AAPT2 mistakenly took this as an error.

Change-Id: I07a2fe9f551daefcce842f205fb219d2fa453ebc

10 files changed, 148 insertions, 83 deletions

diff --git a/tools/aapt2/BinaryResourceParser.cpp b/tools/aapt2/BinaryResourceParser.cpp
index bad5aa5..d16f63b 100644
--- a/tools/aapt2/BinaryResourceParser.cpp
+++ b/tools/aapt2/BinaryResourceParser.cpp
@@ -396,6 +396,12 @@ bool BinaryResourceParser::parsePackage(const ResChunk_header* chunk) {
             }
             break;
 
+        case RES_TABLE_PUBLIC_TYPE:
+            if (!parsePublic(parser.getChunk())) {
+                return false;
+            }
+            break;
+
         default:
             Logger::warn(mSource)
                     << "unexpected chunk of type "
@@ -429,6 +435,55 @@ bool BinaryResourceParser::parsePackage(const ResChunk_header* chunk) {
     return true;
 }
 
+bool BinaryResourceParser::parsePublic(const ResChunk_header* chunk) {
+    const Public_header* header = convertTo<Public_header>(chunk);
+
+    if (header->typeId == 0) {
+        Logger::error(mSource)
+                << "invalid type ID " << header->typeId << std::endl;
+        return false;
+    }
+
+    const ResourceType* parsedType = parseResourceType(util::getString(mTypePool,
+                                                                       header->typeId - 1));
+    if (!parsedType) {
+        Logger::error(mSource)
+                << "invalid type " << util::getString(mTypePool, header->typeId - 1) << std::endl;
+        return false;
+    }
+
+    const uintptr_t chunkEnd = reinterpret_cast<uintptr_t>(chunk) + chunk->size;
+    const Public_entry* entry = reinterpret_cast<const Public_entry*>(
+            getChunkData(header->header));
+    for (uint32_t i = 0; i < header->count; i++) {
+        if (reinterpret_cast<uintptr_t>(entry) + sizeof(*entry) > chunkEnd) {
+            Logger::error(mSource)
+                    << "Public_entry extends beyond chunk."
+                    << std::endl;
+            return false;
+        }
+
+        const ResourceId resId = { mTable->getPackageId(), header->typeId, entry->entryId };
+        const ResourceName name = {
+                mTable->getPackage(),
+                *parsedType,
+                util::getString(mKeyPool, entry->key.index).toString() };
+
+        SourceLine source;
+        if (mSourcePool.getError() == NO_ERROR) {
+            source.path = util::utf16ToUtf8(util::getString(mSourcePool, entry->source.index));
+            source.line = entry->sourceLine;
+        }
+
+        if (!mTable->markPublic(name, resId, source)) {
+            return false;
+        }
+
+        entry++;
+    }
+    return true;
+}
+
 bool BinaryResourceParser::parseTypeSpec(const ResChunk_header* chunk) {
     if (mTypePool.getError() != NO_ERROR) {
         Logger::error(mSource)
@@ -636,10 +691,6 @@ std::unique_ptr<Item> BinaryResourceParser::parseValue(const ResourceNameRef& na
         return util::make_unique<BinaryPrimitive>(nullType);
     }
 
-    if (value->dataType == ExtendedTypes::TYPE_SENTINEL) {
-        return util::make_unique<Sentinel>();
-    }
-
     if (value->dataType == ExtendedTypes::TYPE_RAW_STRING) {
         return util::make_unique<RawString>(
                 mTable->getValueStringPool().makeRef(util::getString(mValuePool, value->data),
diff --git a/tools/aapt2/BinaryResourceParser.h b/tools/aapt2/BinaryResourceParser.h
index 2ac02c9..32876cd 100644
--- a/tools/aapt2/BinaryResourceParser.h
+++ b/tools/aapt2/BinaryResourceParser.h
@@ -66,6 +66,7 @@ private:
     bool idToName(Reference* reference);
 
     bool parsePackage(const android::ResChunk_header* chunk);
+    bool parsePublic(const android::ResChunk_header* chunk);
     bool parseTypeSpec(const android::ResChunk_header* chunk);
     bool parseType(const android::ResChunk_header* chunk);
 
diff --git a/tools/aapt2/Linker.cpp b/tools/aapt2/Linker.cpp
index 42ea0f1..a8b7a14 100644
--- a/tools/aapt2/Linker.cpp
+++ b/tools/aapt2/Linker.cpp
@@ -67,13 +67,21 @@ bool Linker::linkAndValidate() {
                 usedIds[type->typeId].insert(entry->entryId);
             }
 
-            for (auto& valueConfig : entry->values) {
-                // Dispatch to the right method of this linker
-                // based on the value's type.
-                valueConfig.value->accept(*this, Args{
-                        ResourceNameRef{ mTable->getPackage(), type->type, entry->name },
-                        valueConfig.source
-                });
+            if (entry->publicStatus.isPublic && entry->values.empty()) {
+                // A public resource has no values. It will not be encoded
+                // properly without a symbol table. This is a unresolved symbol.
+                addUnresolvedSymbol(ResourceNameRef{
+                        mTable->getPackage(), type->type, entry->name },
+                        entry->publicStatus.source);
+            } else {
+                for (auto& valueConfig : entry->values) {
+                    // Dispatch to the right method of this linker
+                    // based on the value's type.
+                    valueConfig.value->accept(*this, Args{
+                            ResourceNameRef{ mTable->getPackage(), type->type, entry->name },
+                            valueConfig.source
+                    });
+                }
             }
         }
     }
@@ -270,11 +278,6 @@ void Linker::visit(Styleable& styleable, ValueVisitorArgs& a) {
     }
 }
 
-void Linker::visit(Sentinel& sentinel, ValueVisitorArgs& a) {
-    Args& args = static_cast<Args&>(a);
-    addUnresolvedSymbol(args.referrer, args.source);
-}
-
 void Linker::visit(Array& array, ValueVisitorArgs& a) {
     Args& args = static_cast<Args&>(a);
 
diff --git a/tools/aapt2/Linker.h b/tools/aapt2/Linker.h
index d34e487..9db64ab 100644
--- a/tools/aapt2/Linker.h
+++ b/tools/aapt2/Linker.h
@@ -88,7 +88,6 @@ private:
     void visit(Attribute& attribute, ValueVisitorArgs& args) override;
     void visit(Styleable& styleable, ValueVisitorArgs& args) override;
     void visit(Style& style, ValueVisitorArgs& args) override;
-    void visit(Sentinel& sentinel, ValueVisitorArgs& args) override;
     void visit(Array& array, ValueVisitorArgs& args) override;
     void visit(Plural& plural, ValueVisitorArgs& args) override;
 
diff --git a/tools/aapt2/ResourceTable.cpp b/tools/aapt2/ResourceTable.cpp
index 7f55395..9468860 100644
--- a/tools/aapt2/ResourceTable.cpp
+++ b/tools/aapt2/ResourceTable.cpp
@@ -299,16 +299,12 @@ bool ResourceTable::markPublic(const ResourceNameRef& name, const ResourceId res
 
     type->publicStatus.isPublic = true;
     entry->publicStatus.isPublic = true;
+    entry->publicStatus.source = source;
 
     if (resId.isValid()) {
         type->typeId = resId.typeId();
         entry->entryId = resId.entryId();
     }
-
-    if (entry->values.empty()) {
-        entry->values.push_back(ResourceConfigValue{ {}, source, {},
-                                    util::make_unique<Sentinel>() });
-    }
     return true;
 }
 
diff --git a/tools/aapt2/ResourceTable.h b/tools/aapt2/ResourceTable.h
index 3591d11..94bacd8 100644
--- a/tools/aapt2/ResourceTable.h
+++ b/tools/aapt2/ResourceTable.h
@@ -35,6 +35,7 @@ namespace aapt {
  */
 struct Public {
     bool isPublic = false;
+    SourceLine source;
     std::u16string comment;
 };
 
diff --git a/tools/aapt2/ResourceTypeExtensions.h b/tools/aapt2/ResourceTypeExtensions.h
index 60e225e..dcbe923 100644
--- a/tools/aapt2/ResourceTypeExtensions.h
+++ b/tools/aapt2/ResourceTypeExtensions.h
@@ -30,6 +30,8 @@ namespace aapt {
  * future collisions.
  */
 enum {
+    RES_TABLE_PUBLIC_TYPE = 0x000d,
+
     /**
      * A chunk that holds the string pool
      * for source entries (path/to/source:line).
@@ -51,13 +53,6 @@ enum {
 struct ExtendedTypes {
     enum {
         /**
-         * A sentinel value used when a resource is defined as
-         * public but it has no defined value yet. If we don't
-         * flatten it with some value, we will lose its name.
-         */
-        TYPE_SENTINEL = 0xff,
-
-        /**
          * A raw string value that hasn't had its escape sequences
          * processed nor whitespace removed.
          */
@@ -65,6 +60,38 @@ struct ExtendedTypes {
     };
 };
 
+struct Public_header {
+    android::ResChunk_header header;
+
+    /**
+     * The ID of the type this structure refers to.
+     */
+    uint8_t typeId;
+
+    /**
+     * Reserved. Must be 0.
+     */
+    uint8_t res0;
+
+    /**
+     * Reserved. Must be 0.
+     */
+    uint16_t res1;
+
+    /**
+     * Number of public entries.
+     */
+    uint32_t count;
+};
+
+struct Public_entry {
+    uint16_t entryId;
+    uint16_t res0;
+    android::ResStringPool_ref key;
+    android::ResStringPool_ref source;
+    uint32_t sourceLine;
+};
+
 /**
  * A chunk with type RES_TABLE_SYMBOL_TABLE_TYPE.
  * Following the header are count number of SymbolTable_entry
diff --git a/tools/aapt2/ResourceValues.cpp b/tools/aapt2/ResourceValues.cpp
index 3a6d65d..2bf38e4 100644
--- a/tools/aapt2/ResourceValues.cpp
+++ b/tools/aapt2/ResourceValues.cpp
@@ -217,25 +217,6 @@ void BinaryPrimitive::print(std::ostream& out) const {
     }
 }
 
-bool Sentinel::isWeak() const {
-    return true;
-}
-
-bool Sentinel::flatten(android::Res_value& outValue) const {
-    outValue.dataType = ExtendedTypes::TYPE_SENTINEL;
-    outValue.data = 0;
-    return true;
-}
-
-Sentinel* Sentinel::clone(StringPool* /*newPool*/) const {
-    return new Sentinel();
-}
-
-void Sentinel::print(std::ostream& out) const {
-    out << "(sentinel)";
-    return;
-}
-
 Attribute::Attribute(bool w, uint32_t t) : weak(w), typeMask(t) {
 }
 
diff --git a/tools/aapt2/ResourceValues.h b/tools/aapt2/ResourceValues.h
index e3352f3..f8ece6f 100644
--- a/tools/aapt2/ResourceValues.h
+++ b/tools/aapt2/ResourceValues.h
@@ -206,18 +206,6 @@ struct BinaryPrimitive : public BaseItem<BinaryPrimitive> {
     void print(std::ostream& out) const override;
 };
 
-/**
- * Sentinel value that should be ignored in the final output.
- * Mainly used as a placeholder for public entries with no
- * values defined yet.
- */
-struct Sentinel : public BaseItem<Sentinel> {
-    bool isWeak() const override;
-    bool flatten(android::Res_value& outValue) const override;
-    Sentinel* clone(StringPool* newPool) const override;
-    void print(std::ostream& out) const override;
-};
-
 struct Attribute : public BaseValue<Attribute> {
     struct Symbol {
         Reference symbol;
@@ -332,10 +320,6 @@ struct ValueVisitor {
         visitItem(primitive, args);
     }
 
-    virtual void visit(Sentinel& sentinel, ValueVisitorArgs& args) {
-        visitItem(sentinel, args);
-    }
-
     virtual void visit(Attribute& attr, ValueVisitorArgs& args) {}
     virtual void visit(Style& style, ValueVisitorArgs& args) {}
     virtual void visit(Array& array, ValueVisitorArgs& args) {}
@@ -377,10 +361,6 @@ struct ConstValueVisitor {
         visitItem(primitive, args);
     }
 
-    virtual void visit(const Sentinel& sentinel, ValueVisitorArgs& args) {
-        visitItem(sentinel, args);
-    }
-
     virtual void visit(const Attribute& attr, ValueVisitorArgs& args) {}
     virtual void visit(const Style& style, ValueVisitorArgs& args) {}
     virtual void visit(const Array& array, ValueVisitorArgs& args) {}
diff --git a/tools/aapt2/TableFlattener.cpp b/tools/aapt2/TableFlattener.cpp
index 406e506..aa0f1d5 100644
--- a/tools/aapt2/TableFlattener.cpp
+++ b/tools/aapt2/TableFlattener.cpp
@@ -31,8 +31,8 @@
 namespace aapt {
 
 struct FlatEntry {
-    const ResourceEntry& entry;
-    const Value& value;
+    const ResourceEntry* entry;
+    const Value* value;
     uint32_t entryKey;
     uint32_t sourcePathKey;
     uint32_t sourceLine;
@@ -48,10 +48,10 @@ public:
         mMap = mOut->nextBlock<android::ResTable_map_entry>();
         mMap->key.index = flatEntry.entryKey;
         mMap->flags = android::ResTable_entry::FLAG_COMPLEX;
-        if (flatEntry.entry.publicStatus.isPublic) {
+        if (flatEntry.entry->publicStatus.isPublic) {
             mMap->flags |= android::ResTable_entry::FLAG_PUBLIC;
         }
-        if (flatEntry.value.isWeak()) {
+        if (flatEntry.value->isWeak()) {
             mMap->flags |= android::ResTable_entry::FLAG_WEAK;
         }
 
@@ -229,14 +229,14 @@ TableFlattener::TableFlattener(Options options)
 
 bool TableFlattener::flattenValue(BigBuffer* out, const FlatEntry& flatEntry,
                                   SymbolEntryVector* symbols) {
-    if (flatEntry.value.isItem()) {
+    if (flatEntry.value->isItem()) {
         android::ResTable_entry* entry = out->nextBlock<android::ResTable_entry>();
 
-        if (flatEntry.entry.publicStatus.isPublic) {
+        if (flatEntry.entry->publicStatus.isPublic) {
             entry->flags |= android::ResTable_entry::FLAG_PUBLIC;
         }
 
-        if (flatEntry.value.isWeak()) {
+        if (flatEntry.value->isWeak()) {
             entry->flags |= android::ResTable_entry::FLAG_WEAK;
         }
 
@@ -252,14 +252,14 @@ bool TableFlattener::flattenValue(BigBuffer* out, const FlatEntry& flatEntry,
             entry->size += sizeof(*sourceBlock);
         }
 
-        const Item* item = static_cast<const Item*>(&flatEntry.value);
+        const Item* item = static_cast<const Item*>(flatEntry.value);
         ValueFlattener flattener(out, symbols);
         item->accept(flattener, {});
         return flattener.result;
     }
 
     MapFlattener flattener(out, flatEntry, symbols);
-    flatEntry.value.accept(flattener, {});
+    flatEntry.value->accept(flattener, {});
     return true;
 }
 
@@ -373,6 +373,15 @@ bool TableFlattener::flatten(BigBuffer* out, const ResourceTable& table) {
             }
         }
 
+        const size_t beforePublicHeader = typeBlock.size();
+        Public_header* publicHeader = nullptr;
+        if (mOptions.useExtendedChunks) {
+            publicHeader = typeBlock.nextBlock<Public_header>();
+            publicHeader->header.type = RES_TABLE_PUBLIC_TYPE;
+            publicHeader->header.headerSize = sizeof(*publicHeader);
+            publicHeader->typeId = type->typeId;
+        }
+
         // The binary resource table lists resource entries for each configuration.
         // We store them inverted, where a resource entry lists the values for each
         // configuration available. Here we reverse this to match the binary table.
@@ -387,18 +396,35 @@ bool TableFlattener::flatten(BigBuffer* out, const ResourceTable& table) {
                 return false;
             }
 
+            if (publicHeader && entry->publicStatus.isPublic) {
+                // Write the public status of this entry.
+                Public_entry* publicEntry = typeBlock.nextBlock<Public_entry>();
+                publicEntry->entryId = static_cast<uint32_t>(entry->entryId);
+                publicEntry->key.index = static_cast<uint32_t>(keyIndex);
+                publicEntry->source.index = static_cast<uint32_t>(sourcePool.makeRef(
+                            util::utf8ToUtf16(entry->publicStatus.source.path)).getIndex());
+                publicEntry->sourceLine = static_cast<uint32_t>(entry->publicStatus.source.line);
+                publicHeader->count += 1;
+            }
+
             for (const auto& configValue : entry->values) {
                 data[configValue.config].push_back(FlatEntry{
-                        *entry,
-                        *configValue.value,
+                        entry,
+                        configValue.value.get(),
                         static_cast<uint32_t>(keyIndex),
                         static_cast<uint32_t>(sourcePool.makeRef(util::utf8ToUtf16(
-                                        configValue.source.path)).getIndex()),
+                                    configValue.source.path)).getIndex()),
                         static_cast<uint32_t>(configValue.source.line)
                 });
             }
         }
 
+        if (publicHeader) {
+            typeBlock.align4();
+            publicHeader->header.size =
+                    static_cast<uint32_t>(typeBlock.size() - beforePublicHeader);
+        }
+
         // Begin flattening a configuration for the current type.
         for (const auto& entry : data) {
             const size_t typeHeaderStart = typeBlock.size();
@@ -416,13 +442,13 @@ bool TableFlattener::flatten(BigBuffer* out, const ResourceTable& table) {
 
             const size_t entryStart = typeBlock.size();
             for (const FlatEntry& flatEntry : entry.second) {
-                assert(flatEntry.entry.entryId < type->entries.size());
-                indices[flatEntry.entry.entryId] = typeBlock.size() - entryStart;
+                assert(flatEntry.entry->entryId < type->entries.size());
+                indices[flatEntry.entry->entryId] = typeBlock.size() - entryStart;
                 if (!flattenValue(&typeBlock, flatEntry, &symbolEntries)) {
                     Logger::error()
                             << "failed to flatten resource '"
                             << ResourceNameRef {
-                                    table.getPackage(), type->type, flatEntry.entry.name }
+                                    table.getPackage(), type->type, flatEntry.entry->name }
                             << "' for configuration '"
                             << entry.first
                             << "'."