[PATCH] D63518: BitStream reader: propagate errors

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp:619
+// FIXME check that the enum is in range.
+auto Code = static_cast(MaybeCode.get());
+

jfb wrote:
> jfb wrote:
> > bjope wrote:
> > > This has caused problem for our sanitizer tests the last couple of days:
> > > 
> > > ```
> > > FAIL: Extra Tools Unit Tests :: 
> > > clang-doc/./ClangDocTests/BitcodeTest.emitMethodInfoBitcode (20077 of 
> > > 48746)
> > >  TEST 'Extra Tools Unit Tests :: 
> > > clang-doc/./ClangDocTests/BitcodeTest.emitMethodInfoBitcode' FAILED 
> > > 
> > > Note: Google Test filter = BitcodeTest.emitMethodInfoBitcode
> > > [==] Running 1 test from 1 test case.
> > > [--] Global test environment set-up.
> > > [--] 1 test from BitcodeTest
> > > [ RUN  ] BitcodeTest.emitMethodInfoBitcode
> > > /local/repo/bbiswjenk/fem023-eiffel003/workspace/llvm/llvm-master-sanitize/clang-tools-extra/clang-doc/BitcodeReader.cpp:621:13:
> > >  runtime error: load of value 9, which is not a valid value for type 
> > > 'llvm::bitc::FixedAbbrevIDs'
> > > 
> > > ```
> > > 
> > > This was seen when building trunk with clang 6.0.0 and 
> > > LVM_USE_SANITIZER=Undefined
> > > 
> > > ```
> > > cmake -G Ninja '-DLLVM_ENABLE_PROJECTS='\''clang;clang-tools-extra'\''' 
> > > -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON 
> > > '-DCMAKE_CXX_FLAGS='\''-stdlib=libc++'\''' -DCMAKE_C_COMPILER=clang 
> > > -DCMAKE_CXX_COMPILER=clang++ -DLLVM_USE_LINKER=gold 
> > > -DLLVM_USE_SANITIZER=Undefined ../.
> > > -- The C compiler identification is Clang 6.0.0
> > > -- The CXX compiler identification is Clang 6.0.0
> > > ```
> > > 
> > > Afaict we can't cast the read value to FixedAbbrevIDs as we do not know 
> > > yet if it matches one of the values defined in the enum, or if we will 
> > > take the default case.
> > > 
> > > 
> > > A similar switch exist at 
> > > cfe/trunk/lib/Frontend/SerializedDiagnosticReader.cpp:127, but it is 
> > > using a slightly different pattern:
> > > 
> > > ```
> > > unsigned Code;
> > > Code = Res.get();
> > > switch ((llvm::bitc::FixedAbbrevIDs)Code) 
> > > ```
> > > I haven't seen any failures for SerializedDiagnosticReader. So either we 
> > > lack test coverage for that function, or the sanitizer only introduce the 
> > > check when using the static_cast (and declaring Code as an enum) as done 
> > > here.
> > > 
> > That sounds like a pre-existing bug. We should check that the value is in 
> > range before casting. Can you send patches to fix both code locations, and 
> > add test coverage? This code is indeed poorly tested.
> > 
> > Why do the sanitizers catch `static_cast` but not C-style casts?
> To be clear: relying on the `default` case is still UB because there's a cast 
> to the enum type before it occurs.
I made a patch here (assuming the goal would be to keep the cast to the enum, 
and to let the switch cover all enum values): https://reviews.llvm.org/D64262


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63518/new/

https://reviews.llvm.org/D63518



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63518: BitStream reader: propagate errors

[JF Bastien via Phabricator via cfe-commits]

jfb marked an inline comment as done.
jfb added inline comments.



Comment at: clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp:619
+// FIXME check that the enum is in range.
+auto Code = static_cast(MaybeCode.get());
+

jfb wrote:
> bjope wrote:
> > This has caused problem for our sanitizer tests the last couple of days:
> > 
> > ```
> > FAIL: Extra Tools Unit Tests :: 
> > clang-doc/./ClangDocTests/BitcodeTest.emitMethodInfoBitcode (20077 of 48746)
> >  TEST 'Extra Tools Unit Tests :: 
> > clang-doc/./ClangDocTests/BitcodeTest.emitMethodInfoBitcode' FAILED 
> > 
> > Note: Google Test filter = BitcodeTest.emitMethodInfoBitcode
> > [==] Running 1 test from 1 test case.
> > [--] Global test environment set-up.
> > [--] 1 test from BitcodeTest
> > [ RUN  ] BitcodeTest.emitMethodInfoBitcode
> > /local/repo/bbiswjenk/fem023-eiffel003/workspace/llvm/llvm-master-sanitize/clang-tools-extra/clang-doc/BitcodeReader.cpp:621:13:
> >  runtime error: load of value 9, which is not a valid value for type 
> > 'llvm::bitc::FixedAbbrevIDs'
> > 
> > ```
> > 
> > This was seen when building trunk with clang 6.0.0 and 
> > LVM_USE_SANITIZER=Undefined
> > 
> > ```
> > cmake -G Ninja '-DLLVM_ENABLE_PROJECTS='\''clang;clang-tools-extra'\''' 
> > -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON 
> > '-DCMAKE_CXX_FLAGS='\''-stdlib=libc++'\''' -DCMAKE_C_COMPILER=clang 
> > -DCMAKE_CXX_COMPILER=clang++ -DLLVM_USE_LINKER=gold 
> > -DLLVM_USE_SANITIZER=Undefined ../.
> > -- The C compiler identification is Clang 6.0.0
> > -- The CXX compiler identification is Clang 6.0.0
> > ```
> > 
> > Afaict we can't cast the read value to FixedAbbrevIDs as we do not know yet 
> > if it matches one of the values defined in the enum, or if we will take the 
> > default case.
> > 
> > 
> > A similar switch exist at 
> > cfe/trunk/lib/Frontend/SerializedDiagnosticReader.cpp:127, but it is using 
> > a slightly different pattern:
> > 
> > ```
> > unsigned Code;
> > Code = Res.get();
> > switch ((llvm::bitc::FixedAbbrevIDs)Code) 
> > ```
> > I haven't seen any failures for SerializedDiagnosticReader. So either we 
> > lack test coverage for that function, or the sanitizer only introduce the 
> > check when using the static_cast (and declaring Code as an enum) as done 
> > here.
> > 
> That sounds like a pre-existing bug. We should check that the value is in 
> range before casting. Can you send patches to fix both code locations, and 
> add test coverage? This code is indeed poorly tested.
> 
> Why do the sanitizers catch `static_cast` but not C-style casts?
To be clear: relying on the `default` case is still UB because there's a cast 
to the enum type before it occurs.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63518/new/

https://reviews.llvm.org/D63518



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63518: BitStream reader: propagate errors

[JF Bastien via Phabricator via cfe-commits]

jfb marked 2 inline comments as done.
jfb added inline comments.



Comment at: clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp:619
+// FIXME check that the enum is in range.
+auto Code = static_cast(MaybeCode.get());
+

bjope wrote:
> This has caused problem for our sanitizer tests the last couple of days:
> 
> ```
> FAIL: Extra Tools Unit Tests :: 
> clang-doc/./ClangDocTests/BitcodeTest.emitMethodInfoBitcode (20077 of 48746)
>  TEST 'Extra Tools Unit Tests :: 
> clang-doc/./ClangDocTests/BitcodeTest.emitMethodInfoBitcode' FAILED 
> 
> Note: Google Test filter = BitcodeTest.emitMethodInfoBitcode
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from BitcodeTest
> [ RUN  ] BitcodeTest.emitMethodInfoBitcode
> /local/repo/bbiswjenk/fem023-eiffel003/workspace/llvm/llvm-master-sanitize/clang-tools-extra/clang-doc/BitcodeReader.cpp:621:13:
>  runtime error: load of value 9, which is not a valid value for type 
> 'llvm::bitc::FixedAbbrevIDs'
> 
> ```
> 
> This was seen when building trunk with clang 6.0.0 and 
> LVM_USE_SANITIZER=Undefined
> 
> ```
> cmake -G Ninja '-DLLVM_ENABLE_PROJECTS='\''clang;clang-tools-extra'\''' 
> -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON 
> '-DCMAKE_CXX_FLAGS='\''-stdlib=libc++'\''' -DCMAKE_C_COMPILER=clang 
> -DCMAKE_CXX_COMPILER=clang++ -DLLVM_USE_LINKER=gold 
> -DLLVM_USE_SANITIZER=Undefined ../.
> -- The C compiler identification is Clang 6.0.0
> -- The CXX compiler identification is Clang 6.0.0
> ```
> 
> Afaict we can't cast the read value to FixedAbbrevIDs as we do not know yet 
> if it matches one of the values defined in the enum, or if we will take the 
> default case.
> 
> 
> A similar switch exist at 
> cfe/trunk/lib/Frontend/SerializedDiagnosticReader.cpp:127, but it is using a 
> slightly different pattern:
> 
> ```
> unsigned Code;
> Code = Res.get();
> switch ((llvm::bitc::FixedAbbrevIDs)Code) 
> ```
> I haven't seen any failures for SerializedDiagnosticReader. So either we lack 
> test coverage for that function, or the sanitizer only introduce the check 
> when using the static_cast (and declaring Code as an enum) as done here.
> 
That sounds like a pre-existing bug. We should check that the value is in range 
before casting. Can you send patches to fix both code locations, and add test 
coverage? This code is indeed poorly tested.

Why do the sanitizers catch `static_cast` but not C-style casts?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63518/new/

https://reviews.llvm.org/D63518



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63518: BitStream reader: propagate errors

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp:619
+// FIXME check that the enum is in range.
+auto Code = static_cast(MaybeCode.get());
+

This has caused problem for our sanitizer tests the last couple of days:

```
FAIL: Extra Tools Unit Tests :: 
clang-doc/./ClangDocTests/BitcodeTest.emitMethodInfoBitcode (20077 of 48746)
 TEST 'Extra Tools Unit Tests :: 
clang-doc/./ClangDocTests/BitcodeTest.emitMethodInfoBitcode' FAILED 

Note: Google Test filter = BitcodeTest.emitMethodInfoBitcode
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from BitcodeTest
[ RUN  ] BitcodeTest.emitMethodInfoBitcode
/local/repo/bbiswjenk/fem023-eiffel003/workspace/llvm/llvm-master-sanitize/clang-tools-extra/clang-doc/BitcodeReader.cpp:621:13:
 runtime error: load of value 9, which is not a valid value for type 
'llvm::bitc::FixedAbbrevIDs'

```

This was seen when building trunk with clang 6.0.0 and 
LVM_USE_SANITIZER=Undefined

```
cmake -G Ninja '-DLLVM_ENABLE_PROJECTS='\''clang;clang-tools-extra'\''' 
-DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON 
'-DCMAKE_CXX_FLAGS='\''-stdlib=libc++'\''' -DCMAKE_C_COMPILER=clang 
-DCMAKE_CXX_COMPILER=clang++ -DLLVM_USE_LINKER=gold 
-DLLVM_USE_SANITIZER=Undefined ../.
-- The C compiler identification is Clang 6.0.0
-- The CXX compiler identification is Clang 6.0.0
```

Afaict we can't cast the read value to FixedAbbrevIDs as we do not know yet if 
it matches one of the values defined in the enum, or if we will take the 
default case.


A similar switch exist at 
cfe/trunk/lib/Frontend/SerializedDiagnosticReader.cpp:127, but it is using a 
slightly different pattern:

```
unsigned Code;
Code = Res.get();
switch ((llvm::bitc::FixedAbbrevIDs)Code) 
```
I haven't seen any failures for SerializedDiagnosticReader. So either we lack 
test coverage for that function, or the sanitizer only introduce the check when 
using the static_cast (and declaring Code as an enum) as done here.



Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63518/new/

https://reviews.llvm.org/D63518



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63518: BitStream reader: propagate errors

[JF Bastien via Phabricator via cfe-commits]

jfb marked 2 inline comments as done.
jfb added a subscriber: hans.
jfb added inline comments.



Comment at: llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp:205
+  return MaybeBitCode.takeError();
+switch (unsigned BitCode = MaybeBitCode.get()) {
 default: // Default behavior: reject

abrachet wrote:
> This and an identical switch on line 5367 cause an unused variable warning 
> from this commit. I don't know if the build bots report on this, or the 
> proper way to tell you about this but hopefully you will see it :)
Thanks, looks like @Hans fixed it in r364505. Odd that it wasn't warning 
locally for me.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63518/new/

https://reviews.llvm.org/D63518



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63518: BitStream reader: propagate errors

[Alex Brachet via Phabricator via cfe-commits]

abrachet added inline comments.



Comment at: llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp:205
+  return MaybeBitCode.takeError();
+switch (unsigned BitCode = MaybeBitCode.get()) {
 default: // Default behavior: reject

This and an identical switch on line 5367 cause an unused variable warning from 
this commit. I don't know if the build bots report on this, or the proper way 
to tell you about this but hopefully you will see it :)


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63518/new/

https://reviews.llvm.org/D63518



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63518: BitStream reader: propagate errors

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

I think this is ready to go! I rebased and ran `check-all` for LLVM / clang / 
clang-tools-extras and everything passes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63518/new/

https://reviews.llvm.org/D63518



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63518: BitStream reader: propagate errors

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

I've addressed Lang's comments and the re-audited all the new FIXME instances. 
This patch now actually drops errors on the floor instead of implicitly 
erroring out unless the error path is tested (which is what I had before). This 
hides bugs, but I left FIXMEs everywhere and it means the patch will be less 
disruptive because it's bug-compatible in the areas that aren't tested.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63518/new/

https://reviews.llvm.org/D63518



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63518: BitStream reader: propagate errors

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:611-618
+Expected MaybeCode = Stream.ReadCode();
+if (!MaybeCode) {
+  // FIXME this drops the error on the floor.
+  consumeError(MaybeCode.takeError());
+}
+
+// FIXME check that the enum is in range.

jfb wrote:
> lhames wrote:
> > Bigcheese wrote:
> > > Does `consumeError` return?  `MaybeCode.get()` will crash if `!MaybeCode`.
> > Yes: consumeError returns. This should probably be:
> > 
> >   if (!MaybeCode)
> > return consumeError(MaybeCode.takeError()), Cursor::Badness;
> > 
> Good point, fixed.
You tempt me so with this comma operator... but no, I must not!



Comment at: clang/lib/Frontend/CompilerInstance.cpp:2051-2053
+  // FIXME this drops the error on the floor. This code is only used for
+  // typo correction and drops more than just this one source of errors
+  // (such as the directory creation failure above).

lhames wrote:
> This will crash if writeIndex ever generates an error. For that reason I 
> would suggest writing this as:
> 
>   // FIXME: Can actually fail! 
>   cantFail(GlobalModuleIndex::writeIndex(...));
> 
> It has the same effect, but without the control flow.
> 
I think this one shouldn't crash, so I've made it consume the error instead 
(and it still needs to get fixed).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63518/new/

https://reviews.llvm.org/D63518



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63518: BitStream reader: propagate errors

[Francis Visoiu Mistrih via Phabricator via cfe-commits]

thegameg added inline comments.



Comment at: llvm/include/llvm/Bitcode/BitstreamReader.h:489
 
   bool ReadBlockEnd() {
 if (BlockScope.empty()) return true;

jfb wrote:
> thegameg wrote:
> > Any reason why this doesn't return `Error`?
> I'm not sure it's really an error: it goes to the end of the block either 
> because it's empty, or because it pops the scope (which doesn't error 
> either). It might be erroneously used, but I'm not sure we should make it an 
> error right now. WDYT?
Yeah, I'm not sure either... `BitstreamCursor::advance` seems to return 
`BitstreamEntry::getError();` if this function fails after it encountered an 
`END_BLOCK`, and the other users seem to return things like 
`SDError::InvalidDiagnostics` or `Cursor::BadBlock`.

I guess for now, it's fine as it is.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63518/new/

https://reviews.llvm.org/D63518



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63518: BitStream reader: propagate errors

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D63518#1558197 , @lhames wrote:

> I haven't had a chance to audit the whole patch yet, but in general the error 
> suppression idioms are unsafe (though maybe no more so than the existing 
> code?).
>
> I would be inclined to audit all those FIXMEs and replace them with cantFails 
> or consumeErrors. consumeError will generally match existing behavior in 
> places where you were ignoring errors. cantFail will get you aggressive 
> crashes, which can be handy for debugging but not so fun in release code.


I haven't addressed the comments yet, but wanted to respond: yes, it's unsafe 
and I was wondering what folks would rather see, so thanks for bringing it up! 
I indeed only consumed errors that we encountered, and in that sense the code 
isn't bug-compatible with what we had before. Seems like you'd want 
`consumeError` in most places, which I can certainly do.

> Also, if this patch passes the regression tests we need more failure tests. :)

Indeed! That's a pretty terrifying thing... but I'm not signing up to address 
*that* particular issue :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63518/new/

https://reviews.llvm.org/D63518



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63518: BitStream reader: propagate errors

[JF Bastien via Phabricator via cfe-commits]

jfb marked 8 inline comments as done.
jfb added inline comments.



Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:527-529
+  if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
+if (llvm::Error Skipped = Stream.SkipBlock())
+  return Skipped;

lhames wrote:
> Bigcheese wrote:
> > This is a pretty big behavior change.  Is clang-doc expecting to get files 
> > with unknown blocks?
> The inner test here is unsafe, as it will discard the outer error. You need:
> 
>   if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
> if (llvm::Error Skipped = Stream.SkipBlock()) {
>   consumeError(std::move(Err));
>   return Skipped;
> }
> return Err;
>   }
> 
> or:
> 
>   if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
> if (llvm::Error Skipped = Stream.SkipBlock()) {
>   return joinErrors(std::move(Err), std::move(Skipped));
> return Err;
>   }
I don't think it expects this, and it looks (from the above code) like the 
intent is to handle errors when parsing (so this would be a bug that we'd want 
fixed).



Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:611-618
+Expected MaybeCode = Stream.ReadCode();
+if (!MaybeCode) {
+  // FIXME this drops the error on the floor.
+  consumeError(MaybeCode.takeError());
+}
+
+// FIXME check that the enum is in range.

lhames wrote:
> Bigcheese wrote:
> > Does `consumeError` return?  `MaybeCode.get()` will crash if `!MaybeCode`.
> Yes: consumeError returns. This should probably be:
> 
>   if (!MaybeCode)
> return consumeError(MaybeCode.takeError()), Cursor::Badness;
> 
Good point, fixed.



Comment at: llvm/include/llvm/Bitcode/BitstreamReader.h:489
 
   bool ReadBlockEnd() {
 if (BlockScope.empty()) return true;

thegameg wrote:
> Any reason why this doesn't return `Error`?
I'm not sure it's really an error: it goes to the end of the block either 
because it's empty, or because it pops the scope (which doesn't error either). 
It might be erroneously used, but I'm not sure we should make it an error right 
now. WDYT?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63518/new/

https://reviews.llvm.org/D63518



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63518: BitStream reader: propagate errors

[Lang Hames via Phabricator via cfe-commits]

lhames added a comment.

I haven't had a chance to audit the whole patch yet, but in general the error 
suppression idioms are unsafe (though maybe no more so than the existing code?).

I would be inclined to audit all those FIXMEs and replace them with cantFails 
or consumeErrors. consumeError will generally match existing behavior in places 
where you were ignoring errors. cantFail will get you aggressive crashes, which 
can be handy for debugging but not so fun in release code.

Also, if this patch passes the regression tests we need more failure tests. :)




Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:527-529
+  if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
+if (llvm::Error Skipped = Stream.SkipBlock())
+  return Skipped;

Bigcheese wrote:
> This is a pretty big behavior change.  Is clang-doc expecting to get files 
> with unknown blocks?
The inner test here is unsafe, as it will discard the outer error. You need:

  if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
if (llvm::Error Skipped = Stream.SkipBlock()) {
  consumeError(std::move(Err));
  return Skipped;
}
return Err;
  }

or:

  if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
if (llvm::Error Skipped = Stream.SkipBlock()) {
  return joinErrors(std::move(Err), std::move(Skipped));
return Err;
  }



Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:611-618
+Expected MaybeCode = Stream.ReadCode();
+if (!MaybeCode) {
+  // FIXME this drops the error on the floor.
+  consumeError(MaybeCode.takeError());
+}
+
+// FIXME check that the enum is in range.

Bigcheese wrote:
> Does `consumeError` return?  `MaybeCode.get()` will crash if `!MaybeCode`.
Yes: consumeError returns. This should probably be:

  if (!MaybeCode)
return consumeError(MaybeCode.takeError()), Cursor::Badness;




Comment at: clang/lib/Frontend/CompilerInstance.cpp:2051-2053
+  // FIXME this drops the error on the floor. This code is only used for
+  // typo correction and drops more than just this one source of errors
+  // (such as the directory creation failure above).

This will crash if writeIndex ever generates an error. For that reason I would 
suggest writing this as:

  // FIXME: Can actually fail! 
  cantFail(GlobalModuleIndex::writeIndex(...));

It has the same effect, but without the control flow.




Comment at: clang/lib/Frontend/CompilerInstance.cpp:2082
+  getPreprocessor().getHeaderSearchInfo().getModuleCachePath())) {
+// FIXME As above, this drops the error on the floor.
+  }

Another cantFail candidate.



Comment at: clang/lib/Frontend/FrontendAction.cpp:945-948
+// FIXME this drops the error on the floor, but
+// Index/pch-from-libclang.c seems to rely on dropping at least some of
+// the error conditions!
+consumeError(std::move(Err));

I'd suggest cantFail here, if not for that fixme comment. Yikes.



Comment at: clang/lib/Frontend/Rewrite/FrontendActions.cpp:132-133
  CI.getLangOpts(), FixItOpts.get());
-  FixAction->Execute();
+  if (llvm::Error Err = FixAction->Execute())
+err = true; // FIXME this drops the error on the floor.
 

You need a consumeError here or you'll get a runtime crash if an error is 
generated.



Comment at: clang/lib/Frontend/SerializedDiagnosticReader.cpp:89-91
+  if (llvm::Error Err = Stream.SkipBlock())
+return SDError::MalformedTopLevelBlock; // FIXME propagate the error
+// details.

This needs a consumeError or it will crash if an error is generated. How about:

if (llvm::Error Err = Stream.SkipBlock())
  return consumeError(std::move(Err)), SDError::MalformedTopLevelBlock;




Comment at: clang/lib/Frontend/SerializedDiagnosticReader.cpp:149-151
+  if (llvm::Error Err =
+  Stream.EnterSubBlock(clang::serialized_diags::BLOCK_META))
+return SDError::MalformedMetadataBlock; // FIXME propagate the error

Needs a consumeError.



Comment at: clang/lib/Frontend/SerializedDiagnosticReader.cpp:166-167
 case Cursor::BlockBegin:
-  if (Stream.SkipBlock())
-return SDError::MalformedMetadataBlock;
+  if (llvm::Error Err = Stream.SkipBlock())
+return SDError::MalformedMetadataBlock; // FIXME propagate the error
+// details.

Needs a consumeError.



Comment at: clang/lib/Frontend/SerializedDiagnosticReader.cpp:194-196
+  if (llvm::Error Err =
+  Stream.EnterSubBlock(clang::serialized_diags::BLOCK_DIAG))
+return SDError::MalformedDiagnosticBlock; // 

[PATCH] D63518: BitStream reader: propagate errors

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added inline comments.



Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:527-529
+  if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
+if (llvm::Error Skipped = Stream.SkipBlock())
+  return Skipped;

This is a pretty big behavior change.  Is clang-doc expecting to get files with 
unknown blocks?



Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:611-618
+Expected MaybeCode = Stream.ReadCode();
+if (!MaybeCode) {
+  // FIXME this drops the error on the floor.
+  consumeError(MaybeCode.takeError());
+}
+
+// FIXME check that the enum is in range.

Does `consumeError` return?  `MaybeCode.get()` will crash if `!MaybeCode`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63518/new/

https://reviews.llvm.org/D63518



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63518: BitStream reader: propagate errors

[Francis Visoiu Mistrih via Phabricator via cfe-commits]

thegameg added inline comments.



Comment at: llvm/include/llvm/Bitcode/BitstreamReader.h:441
   // If we found a sub-block, just skip over it and check the next entry.
-  if (SkipBlock())
-return BitstreamEntry::getError();
+  if (llvm::Error Err = SkipBlock())
+return std::move(Err);

`llvm::` seems unnecessary here.



Comment at: llvm/include/llvm/Bitcode/BitstreamReader.h:489
 
   bool ReadBlockEnd() {
 if (BlockScope.empty()) return true;

Any reason why this doesn't return `Error`?



Comment at: llvm/lib/Bitcode/Reader/BitstreamReader.cpp:140
 CodeOp.getEncoding() == BitCodeAbbrevOp::Blob)
   report_fatal_error("Abbreviation starts with an Array or a Blob");
+Expected MaybeCode = readAbbreviatedField(*this, CodeOp);

`return createStringError` here too?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63518/new/

https://reviews.llvm.org/D63518



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63518: BitStream reader: propagate errors

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno accepted this revision.
bruno added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63518/new/

https://reviews.llvm.org/D63518



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63518: BitStream reader: propagate errors

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

I did a build of all LLVM monorepo projects, and only LLVM / clang / 
clang-tools-extra are impacted. I therefore ran tests as follows, and they all 
pass:

  rm -rf debug ; mkdir debug && (cd debug && cmake -G Ninja ../llvm 
-DLLVM_ENABLE_ASSERTIONS=ON -DCMAKE_BUILD_TYPE=Debug 
-DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra" && ninja check-all )

I haven't run `clang-format` yet so ignore formatting. Otherwise, this patch is 
ready for review and getting committed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63518/new/

https://reviews.llvm.org/D63518



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits