<div dir="ltr">Is it possible to write a test for this?</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Sep 19, 2013 at 7:33 AM, Tom Honermann <span dir="ltr"><<a href="mailto:thonermann@coverity.com" target="_blank">thonermann@coverity.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Could someone please review/comment/commit this patch?<br>
<br>
Tom.<div><div class="h5"><br>
<br>
On 08/28/2013 06:19 PM, Tom Honermann wrote:<br>
</div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
Attached is a patch to correct a lower bound search error that occurs<br>
when searching for module preprocessing entities within a source range.<br>
<br>
The test case I've been using for this is:<br>
<br>
$ cat t.c<br>
#if __GNUC__<br>
#endif<br>
void f(void) {<br>
}<br>
<br>
The problem I was having was that a query for macro expansions occurring<br>
within the definition of f() (source range 3:1-4:1) was returning the<br>
expansion of __GNUC__ occurring at line 1:5. Clearly, no expansions<br>
should be found for that source range.<br>
<br>
The problem was due to lines 4315-4316 below. The loop progressively<br>
reduces a half open range indicated by [First,First+Count) with PPI<br>
pointing to an element in the middle of that range. Note that PPI is<br>
always dereferenceable - it is not a past-the-end iterator (indeed, it<br>
is unconditionally dereferenced at line 4313).<br>
<br>
If all of the source locations for the preprocessed entities precede the<br>
specified source location, then the loop will exit with PPI pointing to<br>
the last element in the range [pp_begin,pp_end). However, the intention<br>
is that, if all entities precede the specified source location, then PPI<br>
is expected to point to pp_end (a past-the-end iterator) when the loop<br>
exits (see line 4322). When the loop exits with PPI not set to pp_end,<br>
then it is interpreted as pointing to the first preprocessed entity with<br>
a source location following the specified location. This is why my<br>
query is incorrectly including the preceding macro expansion.<br>
<br>
The attached patch against trunk (revision 189518) modifies lines<br>
4315-4316 to first increment PPI, and then to assign First to PPI. This<br>
causes PPI to be advanced appropriately to satisfy the comparison at<br>
line 4322 when all entities precede the specified source location.<br>
<br>
I looked for a simple way to provide a test for this behavior, but I<br>
wasn't able to find one. It seems there may be some way to test this<br>
with c-index-test, but I wasn't able to figure out a way to do so (the<br>
problem only occurs when the relevant code is loaded from a PCH file).<br>
I'm afraid I'm not very familiar with the existing tests. I did run the<br>
Clang test suite (make test) and no regressions were reported.<br>
<br>
lib/Serialization/ASTReader.<u></u>cpp:<br>
4280 /// \brief Returns the first preprocessed entity ID that ends after<br>
\arg BLoc.<br>
4281 PreprocessedEntityID<br>
4282 ASTReader::<u></u>findBeginPreprocessedEntity(<u></u>SourceLocation BLoc) const {<br>
4283 if (SourceMgr.<u></u>isLocalSourceLocation(BLoc))<br>
4284 return getTotalNumPreprocessedEntitie<u></u>s();<br>
4285<br>
4286 GlobalSLocOffsetMapType::<u></u>const_iterator<br>
4287 SLocMapI =<br>
GlobalSLocOffsetMap.find(<u></u>SourceManager::MaxLoadedOffset -<br>
4288 BLoc.getOffset() - 1);<br>
4289 assert(SLocMapI != GlobalSLocOffsetMap.end() &&<br>
4290 "Corrupted global sloc offset map");<br>
4291<br>
4292 if (SLocMapI->second-><u></u>NumPreprocessedEntities == 0)<br>
4293 return findNextPreprocessedEntity(<u></u>SLocMapI);<br>
4294<br>
4295 ModuleFile &M = *SLocMapI->second;<br>
4296 typedef const PPEntityOffset *pp_iterator;<br>
4297 pp_iterator pp_begin = M.PreprocessedEntityOffsets;<br>
4298 pp_iterator pp_end = pp_begin + M.NumPreprocessedEntities;<br>
4299<br>
4300 size_t Count = M.NumPreprocessedEntities;<br>
4301 size_t Half;<br>
4302 pp_iterator First = pp_begin;<br>
4303 pp_iterator PPI;<br>
4304<br>
4305 // Do a binary search manually instead of using std::lower_bound<br>
because<br>
4306 // The end locations of entities may be unordered (when a macro<br>
expansion<br>
4307 // is inside another macro argument), but for this case it is not<br>
important<br>
4308 // whether we get the first macro expansion or its containing macro.<br>
4309 while (Count > 0) {<br>
4310 Half = Count/2;<br>
4311 PPI = First;<br>
4312 std::advance(PPI, Half);<br>
4313 if (SourceMgr.<u></u>isBeforeInTranslationUnit(<u></u>ReadSourceLocation(M,<br>
PPI->End),<br>
4314 BLoc)){<br>
4315 First = PPI;<br>
4316 ++First;<br>
4317 Count = Count - Half - 1;<br>
4318 } else<br>
4319 Count = Half;<br>
4320 }<br>
4321<br>
4322 if (PPI == pp_end)<br>
4323 return findNextPreprocessedEntity(<u></u>SLocMapI);<br>
4324<br>
4325 return M.BasePreprocessedEntityID + (PPI - pp_begin);<br>
4326 }<br>
<br>
Tom.<br>
<br>
<br></div></div>
______________________________<u></u>_________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@cs.uiuc.edu" target="_blank">cfe-dev@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/cfe-dev</a><br>
<br>
</blockquote>
<br>_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@cs.uiuc.edu">cfe-dev@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev</a><br>
<br></blockquote></div><br></div>