lebedev.ri added a comment. Tried fixing `tooling::FrontendActionFactory::create()` in https://reviews.llvm.org/D43779/https://reviews.llvm.org/D43780, but had to revert due to gcc4.8 issues :/
Thank you for working on this, some more review notes. In https://reviews.llvm.org/D41102#1020107, @juliehockett wrote: > In https://reviews.llvm.org/D41102#1017918, @lebedev.ri wrote: > > > Is there some (internal to `BitstreamWriter`) logic that would 'assert()' > > if trying to output some recordid > > which is, according to the `BLOCKINFO_BLOCK`, should not be there? > > E.g. outputting `VERSION` in `BI_COMMENT_BLOCK_ID`? > > > Yes -- it will fail an assertion: > `Assertion 'V == Op.getLiteralValue() && "Invalid abbrev for record!"' > failed.` Ok, great. And it will also complain if you try to output a block within block? ================ Comment at: clang-doc/BitcodeWriter.cpp:184 + return RecordIdNameMap; + } // namespace doc +(); ---------------- That comment seems wrong. **If** the namespace is indeed supposed to be closed, it should happen after the lambda is called, i.e. ``` assert(RecordIdNameMap.size() == RecordIdCount); return RecordIdNameMap; }(); } // namespace doc // AbbreviationMap ================ Comment at: clang-doc/BitcodeWriter.cpp:265 + if (!prepRecordData(ID, !OmitFilenames)) return; + // FIXME: Assert that the line number is of the appropriate size. + Record.push_back(Loc.LineNumber); ---------------- I think it is as simple as ``` assert(Loc.LineNumber < (1U << BitCodeConstants::LineNumberSize)); ``` ? ================ Comment at: clang-doc/BitcodeWriter.cpp:367 + BlockId BID, const std::initializer_list<RecordId> &RIDs) { + emitBlockID(BID); + for (RecordId RID : RIDs) { ---------------- So i guess this should be: ``` void ClangDocBitcodeWriter::emitBlockInfo( BlockId BID, const std::initializer_list<RecordId> &RIDs) { assert(RIDs.size() < (1U << BitCodeConstants::SubblockIDSize), "Too many records in a block!"); emitBlockID(BID); ... ``` ? ================ Comment at: clang-doc/BitcodeWriter.cpp:191 + Record.clear(); + for (const char C : BlockIdNameMap[ID]) Record.push_back(C); + Stream.EmitRecord(llvm::bitc::BLOCKINFO_CODE_BLOCKNAME, Record); ---------------- juliehockett wrote: > lebedev.ri wrote: > > Why do we have this indirection? > > Is there a need to first to (unefficiently?) copy to `Record`, and then > > emit from there? > > Wouldn't this work just as well? > > ``` > > Record.clear(); > > Stream.EmitRecord(llvm::bitc::BLOCKINFO_CODE_BLOCKNAME, BlockIdNameMap[ID]); > > ``` > No, since `BlockIdNameMap[ID]` returns a `StringRef`, which can be > manipulated into an `std::string` or a `const char*`, but the `Stream` wants > an `unsigned char`. So, the copying is to satisfy that. Unless there's a > better way to convert a `StringRef` into an array of `unsigned char`? Aha, i see, did not think of that. But there is a `bytes()` function in `StringRef`, which returns `iterator_range<const unsigned char *>`. Would it help? http://llvm.org/doxygen/classllvm_1_1StringRef.html#a5e8f22c3553e341404b445430a3b075b ================ Comment at: clang-doc/BitcodeWriter.cpp:240 + if (!prepRecordData(ID, Val)) return; + assert(Val < (1U << BitCodeConstants::IntSize)); + Record.push_back(Val); ---------------- juliehockett wrote: > lebedev.ri wrote: > > Ok, now that i think about it, it can't be that easy. > > Maybe > > ``` > > FIXME: assumes 8 bits per byte > > assert(llvm::APInt(8U*sizeof(Val), Val, /*isSigned=*/true).getBitWidth() <= > > BitCodeConstants::IntSize)); > > ``` > > Not sure whether `getBitWidth()` is really the right function to ask though. > > (Not sure how this all works for negative numbers) > That assertion fails :/ I could do something like `static_cast<int64_t>(Val) > == Val` but that would require a) IntSize being a power of 2 b) updating the > assert anytime IntSize is updated, and 3) still throws a warning about > comparing a signed to an unsigned int... I see. Let's not have this assertion for now, just a `FIXME`. ================ Comment at: clang-doc/BitcodeWriter.h:53 + BI_LAST = BI_COMMENT_BLOCK_ID +}; + ---------------- juliehockett wrote: > lebedev.ri wrote: > > juliehockett wrote: > > > lebedev.ri wrote: > > > > So what *exactly* does `BitCodeConstants::SubblockIDSize` mean? > > > > ``` > > > > static_assert(BI_LAST < (1U << BitCodeConstants::SubblockIDSize), "Too > > > > many block id's!"); > > > > ``` > > > > ? > > > It's the current abbrev id width for the block (described [[ > > > https://llvm.org/docs/BitCodeFormat.html#enter-subblock-encoding | here > > > ]]), so it's the max id width for the block's abbrevs. > > So in other words that `static_assert()` is doing the right thing? > > Add it after the `enum BlockId{}` then please, will both document things, > > and ensure that things remain in a sane state. > No...it's the (max) number of the abbrevs relevant to the block itself, which > is to say some subset of the RecordIds for any given block (e.g. for a > `BI_COMMENT_BLOCK`, the number of abbrevs would be 12 and so on the abbrev > width would be 4). > > To assert for it we could put block start/end markers on the RecordIds and > then use that to calculate the bitwidth, if you think the assertion should be > there. Aha, i see, so that should go into `ClangDocBitcodeWriter::emitBlockInfoBlock()`, since that already has that info. (On a related node, it feels like this all should be somehow tablegen-generated, but that is for some later, post-commit cleanup.) https://reviews.llvm.org/D41102 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
