[PATCH] D41102: Setup clang-doc frontend framework

Thu Mar 1 17:10:13 PST 2018

juliehockett added inline comments.

================
Comment at: clang-doc/BitcodeWriter.cpp:196
+/// \brief Emits a record name to the BLOCKINFO block.
+void ClangDocBitcodeWriter::emitRecordID(RecordId ID) {
+  assert(RecordIdNameMap[ID] && "Unknown Abbreviation");
----------------
lebedev.ri wrote:
> juliehockett wrote:
> > lebedev.ri wrote:
> > > Hmm, so i've been staring at this and http://llvm.org/doxygen/classllvm_1_1BitstreamWriter.html and i must say i'm not fond of this indirection.
> > > 
> > > What i don't understand is, in previous function, we don't store `BlockId`, why do we want to store `RecordId`?
> > > Aren't they both unstable, and are implementation detail?
> > > Do we want to store it (`RecordId`)? If yes, please explain it as a new comment in code.
> > > 
> > > If no, i guess this would work too?
> > > ```
> > > assert(RecordIdNameMap[ID] && "Unknown Abbreviation");
> > > Record.clear();
> > > Stream.EmitRecord(llvm::bitc::BLOCKINFO_CODE_SETRECORDNAME, RecordIdNameMap[ID].Name);
> > > ```
> > > And after that you can lower the default size of `SmallVector<> Record` down to, hm, `4`?
> > I'm not entirely certain what you mean -- in `emitBlockId()`, we are storing both the block id and the block name in separate records (`BLOCKINFO_CODE_SETBID`, `BLOCKINFO_CODE_BLOCKNAME`, respectively). In `emitRecordId()`, we're doing something slightly different, in that we emit one record with both the record id and the record name (in record `BLOCKINFO_CODE_SETRECORDNAME`). 
> > 
> > Replacing the copy loop here has the same issue as above, namely that there isn't an easy way to convert between a `StringRef` and an array of `unsigned char`.
> Tried locally, and yes, we do need to output record id.
> 
> What we could **actually** do, is simply inline that `EmitRecord()`, first emitting the RID, and then the name.
> ```
> template <typename Container>
> void EmitRecord(unsigned Code, int ID, const Container &Vals) {
>   // If we don't have an abbrev to use, emit this in its fully unabbreviated
>   // form.
>   auto Count = static_cast<uint32_t>(makeArrayRef(Vals).size());
>   EmitCode(bitc::UNABBREV_RECORD);
>   EmitVBR(Code, 6);
>   EmitVBR(Count + 1, 6); // Including ID
>   EmitVBR64(ID, 6); // 'Prefix' with ID
>   for (unsigned i = 0, e = Count; i != e; ++i)
>     EmitVBR64(Vals[i], 6);
> }
> ```
> 
> But that will result in rather ugly code.
> So given that the record names are quite short, and all the other strings we output directly, maybe leave it as it is for now, until it shows in profiles?
> 
If that makes sense to you, sounds good to me!

================
Comment at: clang-doc/BitcodeWriter.cpp:139
+          {COMMENT_NAME, {"Name", &StringAbbrev}},
+          {COMMENT_POSITION, {"Position", &IntAbbrev}},
+          {COMMENT_DIRECTION, {"Direction", &StringAbbrev}},
----------------
lebedev.ri wrote:
> This change is not covered by tests.
> (I've actually found out that the hard way, by trying to find why it didn't trigger any asssertions, oh well)
So after a some digging, this particular field can't be tested right now as the mapper doesn't look at any `TemplateDecl`s (something that definitely needs to be implemented, but in a follow-on patch). I've removed it for now, until it can be properly used/tested.

================
Comment at: clang-doc/BitcodeWriter.h:37
+  static constexpr unsigned SubblockIDSize = 4U;
+  static constexpr unsigned BoolSize = 1U;
+  static constexpr unsigned IntSize = 16U;
----------------
lebedev.ri wrote:
> juliehockett wrote:
> > lebedev.ri wrote:
> > > Hmm, you build with asserts enabled, right?
> > > I tried testing this, and three tests fail with
> > > ```
> > > clang-doc: /build/llvm/include/llvm/Bitcode/BitstreamWriter.h:122: void llvm::BitstreamWriter::Emit(uint32_t, unsigned int): Assertion `(Val & ~(~0U >> (32-NumBits))) == 0 && "High bits set!"' failed.
> > > ```
> > > ```
> > > Failing Tests (3):
> > >     Clang Tools :: clang-doc/mapper-class-in-function.cpp
> > >     Clang Tools :: clang-doc/mapper-function.cpp
> > >     Clang Tools :: clang-doc/mapper-method.cpp
> > > 
> > >   Expected Passes    : 6
> > >   Unexpected Failures: 3
> > > ```
> > > At least one failure is because of `BoolSize`, so i'd suspect the assertion itself is wrong...
> > I do, and I've definitely seen that one triggered before but it's been because something was off in how the data was being outputted as I was shifting things around. That said, I'm not seeing it in my local build with this diff though -- I'll update it again just to make sure they're in sync.
> I did not retry with updated tree/patch, but i'm quite sure i did hit those asserts.
> My current build line:
> ```
> -DCMAKE_BUILD_TYPE:STRING=RelWithDebInfo
> -DLLVM_BINUTILS_INCDIR:PATH=/usr/include
> -DLLVM_BUILD_TESTS:BOOL=ON
> -DLLVM_ENABLE_ASSERTIONS:BOOL=ON
> -DLLVM_ENABLE_LLD:BOOL=ON
> -DLLVM_ENABLE_PROJECTS:STRING=clang;libcxx;libcxxabi;compiler-rt;lld
> -DLLVM_ENABLE_SPHINX:BOOL=ON
> -DLLVM_ENABLE_WERROR:BOOL=ON
> -DLLVM_PARALLEL_LINK_JOBS:STRING=1
> -DLLVM_TARGETS_TO_BUILD:STRING=X86
> -DLLVM_USE_SANITIZER:STRING=Address 
> ```
> Additional env variables:
> ```
> export MALLOC_CHECK_=3
> export MALLOC_PERTURB_=$(($RANDOM % 255 + 1))
> export ASAN_OPTIONS=abort_on_error=1
> export UBSAN_OPTIONS=print_stacktrace=1
> ```
Figured it out -- the `Reference` struct didn't have default for the enum, and so if it wasn't initialized it was undefined. Should be fixed now.

================
Comment at: test/clang-doc/mapper-enum.cpp:17
+  // CHECK: <IsDefinition abbrevid=7 op0=1/>
+  // CHECK: <TypeBlock NumWords=4 BlockCodeSize=4>
+    // CHECK: <Type abbrevid=4 op0=3 op1=1/> blob data = 'X'
----------------
lebedev.ri wrote:
> Can `TypeBlock` be on the same depth as `VersionBlock`?
> Via `using`/`typename`? If yes, please add such a test.
Not currently -- I'm planning to add that functionality in the future, but right now it ignores typedef or using decls.

https://reviews.llvm.org/D41102